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

Consolidate COSE things to their own area #25

Merged
merged 2 commits into from Mar 12, 2019
Merged

Conversation

@aseigler
Copy link
Collaborator

aseigler commented Mar 2, 2019

No description provided.

@nicksteele nicksteele self-assigned this Mar 12, 2019
@nicksteele nicksteele merged commit a18915c into duo-labs:master Mar 12, 2019
@nicksteele

This comment has been minimized.

Copy link
Collaborator

nicksteele commented Mar 12, 2019

@jordan-wright and I have looked at this and it LGTU. Thanks again!

@AGWA

This comment has been minimized.

Copy link

AGWA commented Dec 24, 2019

This pull request did more than just move code. It also changed the implementation of OKPPublicKeyData.Verify.

The old code:

// Verify Octet Key Pair (OKP) Public Key Signature
func (k *OKPPublicKeyData) Verify(data []byte, sig []byte) (bool, error) {
f := HasherFromCOSEAlg(COSEAlgorithmIdentifier(k.PublicKeyData.Algorithm))
h := f()
h.Write(data)
return ed25519.Verify(k.XCoord, h.Sum(nil), sig), nil
}

The new code:

// Verify Octet Key Pair (OKP) Public Key Signature
func (k *OKPPublicKeyData) Verify(data []byte, sig []byte) (bool, error) {
f := HasherFromCOSEAlg(COSEAlgorithmIdentifier(k.PublicKeyData.Algorithm))
h := f()
h.Write(data)
var oKey eddsa.PublicKey
err := oKey.FromBytes(k.XCoord)
if err != nil {
return false, err
}
return oKey.Verify(h.Sum(nil), sig), nil
}

@aseigler, what was the rationale behind this change?

@aseigler

This comment has been minimized.

Copy link
Collaborator Author

aseigler commented Dec 24, 2019

I honestly don't recall specifically. It's obvious now that the old code would definitely be preferred if it actually works.

@jordan-wright

This comment has been minimized.

Copy link
Contributor

jordan-wright commented Dec 24, 2019

Just a heads up, I have a PR that I will send up today which addresses #56 but in the process reimplements this part of the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.