Add crypto.Signer support for KMS/HSM keys #6

dineshudayakumar · 2026-01-15T18:59:06Z

Summary

Check public key type instead of private key type to support crypto.Signer implementations (GCP KMS, AWS KMS, HSM) that aren't concrete *rsa.PrivateKey or *ecdsa.PrivateKey types
Add fallback JWT signing using crypto.Signer interface for KMS/HSM keys
Update key type validation in GetSigningContext() to check public key type

kmansou · 2026-01-20T14:27:53Z

I removed integration with Reviewable on this repo

samlsp/middleware_test.go

Copilot

Pull request overview

This PR enables support for hardware security modules (HSM) and cloud Key Management Systems (KMS) by allowing the use of crypto.Signer interface implementations instead of requiring concrete *rsa.PrivateKey or *ecdsa.PrivateKey types.

Changes:

Updated key type validation to check public key type instead of private key type, enabling KMS/HSM signer support
Implemented custom JWT signing fallback using crypto.Signer interface for keys that aren't concrete private key types
Added ECDSA signature format conversion from ASN.1 DER to JWT's raw R||S format

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
service_provider.go	Updated key validation to check public key type instead of private key type
samlsp/session_jwt.go	Added custom JWT signing for crypto.Signer implementations with ECDSA signature conversion
samlsp/request_tracker_jwt.go	Added fallback to custom signing for crypto.Signer implementations
samlsp/new.go	Updated default signing method selection to check public key type
samlsp/middleware_test.go	Added comprehensive end-to-end tests using mock crypto.Signer

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

samlsp/session_jwt.go

Copilot · 2026-01-22T20:20:16Z

samlsp/session_jwt.go

+	case *rsa.PrivateKey, *ecdsa.PrivateKey, ed25519.PrivateKey:
+		return token.SignedString(c.Key)
+	default:
+		return signJWTWithCryptoSigner(token, c.Key, c.SigningMethod)


The type switch checks for concrete private key types that the jwt library can handle, but doesn't verify that c.Key implements crypto.Signer before falling through to signJWTWithCryptoSigner. Consider adding a type assertion to crypto.Signer in the default case to provide a clearer error message if an unsupported key type is passed.

Suggested change

return signJWTWithCryptoSigner(token, c.Key, c.SigningMethod)

signer, ok := c.Key.(crypto.Signer)

if !ok || signer == nil {

return "", fmt.Errorf("unsupported key type %T: expected a non-nil crypto.Signer", c.Key)

}

return signJWTWithCryptoSigner(token, signer, c.SigningMethod)

This check is not required as the c.key is defined as a crypto.signer and golang type system will make sure of that. Doing that will be redundant

The type system doesn't enforce that c.Key is non-nil. If there's a check somewhere that would catch that, that's fine, but it's probably best to return an error instead of allowing a potential nil dereference panic.

What really matters is whether your change would cause a downgrade. If previously the user would not encounter a panic when providing a nil key/signer and after your changes the user would encounter a panic, that is not okay. If, on the other hand, a user would have encountered a panic before your changes, it's acceptable for them to encounter a panic after your changes.

ok, added the nil check

Copilot · 2026-01-22T20:20:16Z

samlsp/request_tracker_jwt.go

+	// Check if key is a concrete private key type that jwt library can handle directly.
+	// For crypto.Signer implementations (KMS/HSM), use custom signing.
+	switch s.Key.(type) {
+	case *rsa.PrivateKey, *ecdsa.PrivateKey, ed25519.PrivateKey:


The type switch checks for concrete private key types that the jwt library can handle, but doesn't verify that s.Key implements crypto.Signer before falling through to signJWTWithCryptoSigner. Consider adding a type assertion to crypto.Signer in the default case to provide a clearer error message if an unsupported key type is passed.

This check is not required as the c.key is defined as a crypto.signer and golang type system will make sure of that. Doing that will be redundant

The type system doesn't enforce that c.Key is non-nil. If there's a check somewhere that would catch that, that's fine, but it's probably best to return an error instead of allowing a potential nil dereference panic.

What really matters is whether your change would cause a downgrade. If previously the user would not encounter a panic when providing a nil key/signer and after your changes the user would encounter a panic, that is not okay. If, on the other hand, a user would have encountered a panic before your changes, it's acceptable for them to encounter a panic after your changes.

added the nil check

eriksw · 2026-01-23T16:38:27Z

service_provider.go

-		if _, ok := sp.Key.(*ecdsa.PrivateKey); !ok {
-			return nil, fmt.Errorf("signature method %s requires a key of type ecdsa.PrivateKey, not %T", sp.SignatureMethod, sp.Key)
+		if _, ok := pubKey.(*ecdsa.PublicKey); !ok {
+			return nil, fmt.Errorf("signature method %s requires an ECDSA key, got %T", sp.SignatureMethod, pubKey)


This is a minor nit, but the old error messages used to specify the expected types for the public keys. That is useful and should probably be preserved.

I reverted back to old error message

eriksw · 2026-01-23T16:38:55Z

samlsp/session_jwt.go

+	case *rsa.PrivateKey, *ecdsa.PrivateKey, ed25519.PrivateKey:
+		return token.SignedString(c.Key)
+	default:
+		return signJWTWithCryptoSigner(token, c.Key, c.SigningMethod)


The type system doesn't enforce that c.Key is non-nil. If there's a check somewhere that would catch that, that's fine, but it's probably best to return an error instead of allowing a potential nil dereference panic.

What really matters is whether your change would cause a downgrade. If previously the user would not encounter a panic when providing a nil key/signer and after your changes the user would encounter a panic, that is not okay. If, on the other hand, a user would have encountered a panic before your changes, it's acceptable for them to encounter a panic after your changes.

eriksw · 2026-01-23T16:42:30Z

samlsp/request_tracker_jwt.go

+	// Check if key is a concrete private key type that jwt library can handle directly.
+	// For crypto.Signer implementations (KMS/HSM), use custom signing.
+	switch s.Key.(type) {
+	case *rsa.PrivateKey, *ecdsa.PrivateKey, ed25519.PrivateKey:


The type system doesn't enforce that c.Key is non-nil. If there's a check somewhere that would catch that, that's fine, but it's probably best to return an error instead of allowing a potential nil dereference panic.

What really matters is whether your change would cause a downgrade. If previously the user would not encounter a panic when providing a nil key/signer and after your changes the user would encounter a panic, that is not okay. If, on the other hand, a user would have encountered a panic before your changes, it's acceptable for them to encounter a panic after your changes.

Check public key type instead of private key type to support crypto.Signer implementations (GCP KMS, AWS KMS, HSM) that aren't concrete *rsa.PrivateKey or *ecdsa.PrivateKey types. Changes: - samlsp/new.go: Update defaultSigningMethodForKey() - samlsp/session_jwt.go: Add fallback signing with crypto.Signer - samlsp/request_tracker_jwt.go: Add fallback signing - service_provider.go: Update GetSigningContext() validation

dineshudayakumar force-pushed the crypto-signer-support branch 2 times, most recently from 3b7da22 to 4a22109 Compare January 15, 2026 20:05

dineshudayakumar force-pushed the crypto-signer-support branch 3 times, most recently from 504881c to d878cf8 Compare January 20, 2026 20:12

eriksw reviewed Jan 20, 2026

View reviewed changes

samlsp/middleware_test.go Show resolved Hide resolved

dineshudayakumar requested a review from eriksw January 22, 2026 19:43

eriksw assigned Copilot Jan 22, 2026

eriksw requested a review from Copilot January 22, 2026 20:19

Copilot AI reviewed Jan 22, 2026

View reviewed changes

dineshudayakumar force-pushed the crypto-signer-support branch from d878cf8 to 9ad3a32 Compare January 23, 2026 16:28

eriksw reviewed Jan 23, 2026

View reviewed changes

dineshudayakumar force-pushed the crypto-signer-support branch from 9ad3a32 to 9f655f0 Compare January 23, 2026 16:55

dineshudayakumar force-pushed the crypto-signer-support branch from 9f655f0 to 9399db6 Compare January 23, 2026 17:00

-		return signJWTWithCryptoSigner(token, c.Key, c.SigningMethod)
+		signer, ok := c.Key.(crypto.Signer)
+		if !ok || signer == nil {
+			return "", fmt.Errorf("unsupported key type %T: expected a non-nil crypto.Signer", c.Key)
+		}
+		return signJWTWithCryptoSigner(token, signer, c.SigningMethod)

Add crypto.Signer support for KMS/HSM keys #6

Are you sure you want to change the base?

Add crypto.Signer support for KMS/HSM keys #6

Uh oh!

Conversation

dineshudayakumar commented Jan 15, 2026 • edited by kmansou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

kmansou commented Jan 20, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dineshudayakumar commented Jan 15, 2026 •

edited by kmansou

Loading