-
Notifications
You must be signed in to change notification settings - Fork 135
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
Implement NewJwtSigner and CreateAttestation #579
base: master
Are you sure you want to change the base?
Changes from 4 commits
eb317a2
abb84ee
ddd21e6
03f1b59
4a2807b
643fe9d
5df1560
237d9f7
960a6b8
f7a29da
13fbc74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -254,3 +254,47 @@ func TestVerifyDetached(t *testing.T) { | |
}) | ||
} | ||
} | ||
|
||
func TestCreateDetachedSignature(t *testing.T) { | ||
tcs := []struct { | ||
name string | ||
key []byte | ||
alg SignatureAlgorithm | ||
expectedError bool | ||
}{ | ||
{ | ||
name: "create rsa signature success", | ||
key: []byte(rsa2048PrivateKey), | ||
alg: RsaSignPkcs12048Sha256, | ||
expectedError: false, | ||
}, { | ||
name: "create ecdsa signature success", | ||
key: []byte(ec256PrivateKey), | ||
alg: EcdsaP256Sha256, | ||
expectedError: false, | ||
}, { | ||
name: "unknown singature algorithm", | ||
key: []byte(rsa2048PrivateKey), | ||
alg: UnknownSigningAlgorithm, | ||
expectedError: true, | ||
}, | ||
} | ||
for _, tc := range tcs { | ||
t.Run(tc.name, func(t *testing.T) { | ||
privKey, err := parsePkixPrivateKeyPem(tc.key) | ||
if err != nil { | ||
t.Fatalf("failed to parse private key %v", err) | ||
} | ||
_, err = createDetachedSignature(privKey, []byte(payload), tc.alg) | ||
if tc.expectedError { | ||
if err == nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: spacing around = There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
t.Errorf("createDetachedSignature(...)=nil, expected non-nil") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit confusing, can be understood as signature is nil. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. discussed during sync. |
||
} | ||
} else { | ||
if err != nil { | ||
t.Errorf("createDetachedSignature(...)=%v, expected nil", err) | ||
} | ||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,65 @@ func getAlgName(alg SignatureAlgorithm) string { | |
} | ||
} | ||
|
||
type jwtSigner struct { | ||
privateKey interface{} | ||
publicKeyID string | ||
signatureAlgorithm SignatureAlgorithm | ||
} | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
// TODO: Explain formatting of JWT private keys. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. name or bug/github issue with all TODOs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
func NewJwtSigner(privateKey []byte, publicKeyID string, alg SignatureAlgorithm) (Signer, error) { | ||
key, err := parsePkixPrivateKeyPem(privateKey) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "error parsing private key") | ||
} | ||
|
||
// If no ID is provided one is computed based on the default digest-based URI extracted from the public key material | ||
if len(publicKeyID) == 0 { | ||
publicKeyID, err = generatePkixPublicKeyId(key) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "error generating public key id") | ||
} | ||
} | ||
return &jwtSigner{ | ||
privateKey: key, | ||
publicKeyID: publicKeyID, | ||
signatureAlgorithm: alg, | ||
}, nil | ||
} | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. payload -> JsonJwtBody, with comment explaining what that is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
type headerTemplate struct { | ||
typ, alg, kid string | ||
} | ||
header := headerTemplate{ | ||
typ: "JWT", | ||
alg: getAlgName(s.signatureAlgorithm), | ||
kid: s.publicKeyID, | ||
} | ||
|
||
headerJson, err := json.Marshal(header) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "error marshaling header") | ||
} | ||
headerBase64 := base64.RawURLEncoding.EncodeToString(headerJson) | ||
payloadBase64 := base64.RawURLEncoding.EncodeToString(payload) | ||
signature, err := createDetachedSignature(s.privateKey, []byte(headerBase64+"."+payloadBase64), s.signatureAlgorithm) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "error creating signature") | ||
} | ||
signatureBase64 := base64.RawURLEncoding.EncodeToString(signature) | ||
jwt := headerBase64 + "." + payloadBase64 + "." + signatureBase64 | ||
return &Attestation{ | ||
PublicKeyID: s.publicKeyID, | ||
Signature: []byte(jwt), | ||
SerializedPayload: payload, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from attestation.go: // SerializedPayload stores the payload over which the signature was 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was a mistake. I updated it to leave the SerializedPayload field empty. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
}, nil | ||
} | ||
|
||
func checkHeader(headerIn []byte, publicKey PublicKey) error { | ||
type headerTemplate struct { | ||
Typ, Alg, Kid, Crit string | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,10 +96,69 @@ func TestVerifyJWT(t *testing.T) { | |
} | ||
} else { | ||
if err != nil { | ||
t.Errorf("Unexpected error: %e", err) | ||
t.Errorf("Unexpected error: %v", err) | ||
} | ||
|
||
} | ||
}) | ||
} | ||
} | ||
|
||
func TestNewJwtSigner(t *testing.T) { | ||
tcs := []struct { | ||
name string | ||
key []byte | ||
publicKeyId string | ||
alg SignatureAlgorithm | ||
expectedError bool | ||
}{ | ||
{ | ||
name: "new jwt singer success", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: singer->signer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
key: []byte(rsa2048PrivateKey), | ||
publicKeyId: "kid", | ||
alg: RsaSignPkcs12048Sha256, | ||
expectedError: false, | ||
}, | ||
{ | ||
name: "new jwt singer with no key id success", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same typo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
key: []byte(rsa2048PrivateKey), | ||
publicKeyId: "", | ||
alg: RsaSignPkcs12048Sha256, | ||
expectedError: false, | ||
}, | ||
{ | ||
name: "new jwt singer with bad key fails", | ||
key: []byte("some-key"), | ||
publicKeyId: "", | ||
alg: RsaSignPkcs12048Sha256, | ||
expectedError: true, | ||
}, | ||
} | ||
for _, tc := range tcs { | ||
t.Run(tc.name, func(t *testing.T) { | ||
_, err := NewJwtSigner(tc.key, tc.publicKeyId, tc.alg) | ||
if tc.expectedError { | ||
if err == nil { | ||
t.Errorf("NewJwtSigner(...) = nil, expected non nil") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: spacing around = Also ditto as above, would this be confusing as to which return value it is referring to? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
} | ||
} else { | ||
if err != nil { | ||
t.Errorf("NewJwtSigner(...)=%v, expected nil", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: spacing around = There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
} | ||
} | ||
}) | ||
} | ||
} | ||
|
||
func TestCreateJwtAttestation(t *testing.T) { | ||
signer, err := NewJwtSigner([]byte(rsa2048PrivateKey), "kid", RsaSignPkcs12048Sha256) | ||
if err != nil { | ||
t.Fatalf("failed to create signer") | ||
} | ||
attestation, err := signer.CreateAttestation([]byte(payload)) | ||
if err != nil { | ||
t.Errorf("CreateAttestation(..) = %v, expected nil", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: spacing around = There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
} else if attestation.PublicKeyID != "kid" { | ||
t.Errorf("attestation.PublicKeyID = %v, expected kid", attestation.PublicKeyID) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: spacing around = There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add two bad cases where alg does not match the key passed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.