Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[python] Pass bytes to cryptography.io's RSAPrivateKey.sign() #1732

Merged
merged 1 commit into from Jan 29, 2024

Conversation

woju
Copy link
Member

@woju woju commented Jan 25, 2024

Description of the changes

See commit message.

Fixes: #1731

How to test this PR?

Check the reproducer in #1731.
Cc: @andre-w-fischer


This change is Reviewable

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)


-- commits line 10 at r1:
FYI: You can see this commit here: pyca/cryptography@0000b40

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @woju)


-- commits line 5 at r1:
-> Technically

Code quote:

Techically

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @woju)

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kailun-qin)


-- commits line 5 at r1:

Previously, kailun-qin (Kailun Qin) wrote…

-> Technically

Done


-- commits line 10 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

FYI: You can see this commit here: pyca/cryptography@0000b40

FYI: I expanded this sentence a bit to make it clearer what commit is meant here.

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion

a discussion (no related file):
Upstream accepted bug and fixed this:

Now I'm not sure if we want to merge this on our side. I'd argue we can, because .to_bytes() should return bytes, but right now there is no technical reason to merge. At least we can reword the commit message.


In versions 42.0.0 and 42.0.1, cryptography.io accepted only bytes for
arguments in various methods like `RSAPrivateKey.sign()`. This bug was
fixed in v42.0.2, but let's fix Gramine to supply bytes instead of
bytearray, so that `Sigstruct.to_bytes()` doesn't fail on those affected
versions of cryptography.io.

Corresponding upstream commit in `pyca/cryptography` repository that
introduced the bug: 0000b402dd5edffa6a86ca560464e83a66fd91f5, and commit
that fixed this bug: 08b24d87a64734ac7f5c575b309ad7d49c246353

Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @woju)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

Upstream accepted bug and fixed this:

Now I'm not sure if we want to merge this on our side. I'd argue we can, because .to_bytes() should return bytes, but right now there is no technical reason to merge. At least we can reword the commit message.

Let me change the commit message slightly. Our fix makes sense to me, and it's better to be on the safe side (what if someone has 42.0.0 - 42.0.1 on their platform).


Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @woju)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Let me change the commit message slightly. Our fix makes sense to me, and it's better to be on the safe side (what if someone has 42.0.0 - 42.0.1 on their platform).

@woju I changed the commit message. Can you check if this reads well to you?


Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@woju I changed the commit message. Can you check if this reads well to you?

I meant, the main thing that we're fixing is, to_bytes() does not return bytes, but bytearray, which is confusing. After next week no one will ever see the affected versions. But whatever, this is also OK, I'm +1-ing.


Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dimakuv)

a discussion (no related file):

to_bytes() does not return bytes, but bytearray, which is confusing

I'm in favor of merging this PR just because of this - the current code is just confusing, so why not fix it up.


Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dimakuv dimakuv merged commit 5286f20 into master Jan 29, 2024
18 checks passed
@dimakuv dimakuv deleted the woju/fix-cryptography-bytes branch January 29, 2024 07:25
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.

gramine-sgx-sign reports "'bytearray' object cannot be converted to 'PyBytes'"
4 participants