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

Adds verification of RSA signatures #26

Merged
merged 9 commits into from
Jul 29, 2021
Merged

Adds verification of RSA signatures #26

merged 9 commits into from
Jul 29, 2021

Conversation

kritzcreek
Copy link
Contributor

No description provided.

src/IC/Crypto/WebAuthn.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

Phone review, might have missed stuff. Most prominently lacking is ic-ref-test use of the new scheme.

src/IC/Crypto.hs Outdated Show resolved Hide resolved
src/IC/Crypto/WebAuthn.hs Outdated Show resolved Hide resolved
[ (TInt 1, TInt 3)
, (TInt 3, TInt (-257))
, (TInt (-1), TBytes (EC.i2osp n))
, (TInt (-2), TBytes (EC.i2osp e))
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want to use EC.i2ospOf_ 32 as above, as otherwise I fear it might do the wrong things if there are leading zeros.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I commented on these lines earlier:

From section 4: https://datatracker.ietf.org/doc/html/rfc8230#section-4

All numeric key parameters are encoded in an unsigned big-endian
representation as an octet sequence using the CBOR byte string
type (major type 2). The octet sequence MUST utilize the minimum
number of octets needed to represent the value.

According to https://hackage.haskell.org/package/cryptonite-0.29/docs/Crypto-PubKey-RSA.html#v:generateWith common choices for e are 0x10001 or 3

which makes me doubt 32bytes "utilizes the minimum number of octets needed to represent the value"

Co-authored-by: Joachim Breitner <[email protected]>
src/IC/Test/WebAuthn.hs Outdated Show resolved Hide resolved
src/IC/Crypto/WebAuthn.hs Outdated Show resolved Hide resolved
@kritzcreek
Copy link
Contributor Author

@nomeata I think I adressed your feedback. PTAL

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -72,94 +77,122 @@ genClientDataJson challenge = JSON.encode $ JSON.Object $
<> "type" JSON..= ("webauthn.get" :: T.Text)
<> "origin" JSON..= ("ic-ref-test" :: T.Text)

parseCOSEKey :: BS.ByteString -> Either T.Text EC.PublicKey
parseCOSEKey s = do
verifyCOSESig :: BS.ByteString -> BS.ByteString -> BS.ByteString -> Either T.Text ()
Copy link

Choose a reason for hiding this comment

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

Any chance we can name these types properly so the signature becomes more meaningful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Sorry for the late response, I was on PTO the last two days)

This matches the already exported signature of verify, and it's a module internal function. If we wanted to go with better names we should probably start at verify.

I think part of the reason we're dealing with lots of ByteStrings in ic-ref's API is that it's primarily a tool used for testing, so it wants to be low-friction for putting together a couple of testcases, rather than having to construct via various newtype wrappers.

@kritzcreek kritzcreek merged commit c49a2f4 into master Jul 29, 2021
@kritzcreek kritzcreek deleted the rsa branch July 29, 2021 16: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.

4 participants