Implement NewJwtSigner and CreateAttestation#579
Implement NewJwtSigner and CreateAttestation#579treyridley wants to merge 11 commits intografeas:masterfrom
Conversation
|
|
||
| // NewJwtSigner creates a Signer interface for JWT Attestations. `publicKeyID` | ||
| // is the ID of the public key that can verify the Attestation signature. | ||
| // TODO: Explain formatting of JWT private keys. |
There was a problem hiding this comment.
name or bug/github issue with all TODOs.
| } | ||
|
|
||
| // NewJwtSigner creates a Signer interface for JWT Attestations. `publicKeyID` | ||
| // is the ID of the public key that can verify the Attestation signature. |
There was a problem hiding this comment.
Move "publicKeyID" to be the last arguement, and in comment say that it should normally be left empty.
Alternatively, I like the idea of having two functions "NewJwtSigner" and "NewJwtSignerExplcitKeyId" , and have the former generate kid and call the latter.
| } | ||
|
|
||
| // CreateAttestation creates a signed JWT Attestation. See Signer for more details. | ||
| func (s *jwtSigner) CreateAttestation(payload []byte) (*Attestation, error) { |
There was a problem hiding this comment.
payload -> JsonJwtBody, with comment explaining what that is.
| return &Attestation{ | ||
| PublicKeyID: s.publicKeyID, | ||
| Signature: []byte(jwt), | ||
| SerializedPayload: payload, |
There was a problem hiding this comment.
from attestation.go: // SerializedPayload stores the payload over which the signature was
// signed. This field is only used for PKIX Attestations.
This is not accurate in the case of JWTs as written now. I think the cleanest way to handle it is to leave SerializedPayload empty for JWTs and update the documentation to reflect that
There was a problem hiding this comment.
this was a mistake. I updated it to leave the SerializedPayload field empty.
| } | ||
| } else { | ||
| if err != nil { | ||
| t.Errorf("NewJwtSigner(...)=%v, expected nil", err) |
| } | ||
| _, err = createDetachedSignature(privKey, []byte(payload), tc.alg) | ||
| if tc.expectedError { | ||
| if err == nil { |
ooq
left a comment
ooq
left a comment
There was a problem hiding this comment.
Thanks for the PR. Left some comments.
| alg: RsaSignPkcs12048Sha256, | ||
| expectedError: false, | ||
| }, { | ||
| name: "create ecdsa signature success", |
There was a problem hiding this comment.
Can you add two bad cases where alg does not match the key passed?
| _, err = createDetachedSignature(privKey, []byte(payload), tc.alg) | ||
| if tc.expectedError { | ||
| if err == nil { | ||
| t.Errorf("createDetachedSignature(...)=nil, expected non-nil") |
There was a problem hiding this comment.
This is a bit confusing, can be understood as signature is nil.
There was a problem hiding this comment.
discussed during sync.
| expectedError bool | ||
| }{ | ||
| { | ||
| name: "new jwt singer success", |
| expectedError: false, | ||
| }, | ||
| { | ||
| name: "new jwt singer with no key id success", |
| } | ||
| attestation, err := signer.CreateAttestation([]byte(payload)) | ||
| if err != nil { | ||
| t.Errorf("CreateAttestation(..) = %v, expected nil", err) |
| _, err := NewJwtSigner(tc.key, tc.alg, tc.publicKeyId) | ||
| if tc.expectedError { | ||
| if err == nil { | ||
| t.Errorf("NewJwtSigner(...) = nil, expected non nil") |
There was a problem hiding this comment.
nit: spacing around =
Also ditto as above, would this be confusing as to which return value it is referring to?
| } | ||
| } else { | ||
| if err != nil { | ||
| t.Errorf("NewJwtSigner(...) = %v, expected nil", err) |
| if err != nil { | ||
| t.Errorf("CreateAttestation(..) = %v, expected nil", err) | ||
| } else if attestation.PublicKeyID != "kid" { | ||
| t.Errorf("attestation.PublicKeyID = %v, expected kid", attestation.PublicKeyID) |
|
Should we merge this? @treyridley |
I changed this to a draft until it is decided if we are going to continue with JWTs. |
4 participants
No description provided.