Skip to content

[WIP] Remove dynamic constants from cryptography products#2556

Closed
vepkenez wants to merge 4 commits intonucypher:mainfrom
vepkenez:constant-signature-sorrow
Closed

[WIP] Remove dynamic constants from cryptography products#2556
vepkenez wants to merge 4 commits intonucypher:mainfrom
vepkenez:constant-signature-sorrow

Conversation

@vepkenez
Copy link
Contributor

@vepkenez vepkenez commented Feb 7, 2021

Type of PR:

  • prevents bugs from the future from ever existing
  • Feature
  • Documentation
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:
Constant Sorrow constants are not really constant in the sense that bytestrings produced by our code may outlive some of the mechanisms by which Constant Sorrow produces the literals for them whether due to encoding, hashlib changes etc. This PR makes them truly static and constant.

It also makes parallel implementation of them in other languages easier in that it is now a copy and paste of some characters instead of a re-implementation of Constant Sorrow magic, or maybe even opens the possibility for pulling from definitions for these bytestrings from a single repo that is accessible to all language implementations.

Issues fixed/closed:

@cygnusv
Copy link
Member

cygnusv commented Feb 7, 2021

Thanks for doing this. I guess these constants should have never live the runtime environment and be serialized anywhere. IMHO, if there's a need to serialize a constant, then this is a hint that you should not use Constant Sorrow for it.

Copy link
Contributor Author

@vepkenez vepkenez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, if there's a need to serialize a constant, then this is a hint that you should not use Constant Sorrow for it.

I was actually thinking we could subclass constant sorrow so it raises an exception if you call __bytes__

# SECP256K1
CAPSULE_LENGTH = 98
PUBLIC_KEY_LENGTH = 33

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KPrasch @cygnusv @derekpierre @fjarri @jMyles
These could be anything.
Currently I made them exactly what Constant Sorrow was outputting so they don't break compatibility with existing bytestrings, but now is the time to break that in favor of a better future.

So what should we make them?

They could be like:
DO_NOT_SIGN = b'DO_NOT_SIGN'

any reason not to do that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just make them sequential bytes? \x00, \x01 and so on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would definitely make cross platform implementing easiest

@KPrasch KPrasch marked this pull request as draft February 7, 2021 23:07
@vepkenez
Copy link
Contributor Author

vepkenez commented Feb 8, 2021

One more consideration before I finish this.

Our encryption looks like this: pre.encrypt(recipient_pubkey_enc, sig_header + plaintext)

We are adding this "signature header" to the actual plaintext.
The signature header was 8 bytes, now we are going to go with single bytes.

But why are we adding this stuff to the plaintext at all? Isn't it kind of a hack?
Would it be better for payload signature to be handled at the app layer?
Would you even trust NuCypher's code to sign your data if your data was so important?

I as an implementer might prefer my own signing mechanics on the the payload anyway and hand the data over to Enrico already signed.

Any support for removing signatures entirely?

@fjarri
Copy link
Contributor

fjarri commented Feb 8, 2021

For what it's worth, I agree with removing the signatures. I always thought of Nucypher as KMS, while ciphertexts are distributed via side channels, so, as you said, it's up to the application to decide whether to sign them or not. But I may be missing some attack vectors here - @tuxxy , @cygnusv ?

@KPrasch KPrasch changed the title Remove dynamic constants from cryptography products [WIP] Remove dynamic constants from cryptography products Feb 22, 2021
@fjarri fjarri mentioned this pull request Jul 10, 2021
@KPrasch
Copy link
Member

KPrasch commented Aug 23, 2021

I'd love to see this PR finished, it's a nice prerequisite to our future plans with entity serialization.

@KPrasch
Copy link
Member

KPrasch commented Sep 20, 2021

Closing this PR for due to staleness. Leaving #1484 open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't serialize Constant Sorrow constants

4 participants