Conversation
…56 support, JWKS endpoint" Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new machine-to-machine OAuth2 authentication module (auth.m2m) to the workflow engine, extending the existing auth plugin beyond browser-based OAuth2 to support service-to-service/API auth.
Changes:
- Introduces
module/M2MAuthModuleimplementingAuthProvider, issuing HS256/ES256 JWTs and serving/oauth/token+/oauth/jwks. - Registers the new module in the auth plugin (factory + UI schema) and updates wfctl’s static module type registry.
- Adds extensive unit tests for M2M grant handling, JWKS/key round-trips, and token verification.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
plugins/auth/plugin.go |
Registers auth.m2m in manifest, adds factory wiring and UI schema for M2M auth configuration. |
plugins/auth/plugin_test.go |
Updates plugin tests for the additional module type and schema. |
module/auth_m2m.go |
Implements the new M2M OAuth2 module: token issuance, JWKS, assertion validation, and AuthProvider verification. |
module/auth_m2m_test.go |
Adds comprehensive tests for grant flows, JWKS behavior, and signing/verification across HS256/ES256. |
cmd/wfctl/type_registry.go |
Adds auth.m2m to the CLI’s known module type registry for config validation. |
module/auth_m2m.go
Outdated
| _ = json.NewEncoder(w).Encode(map[string]string{"error": "JWKS not available for HS256 (symmetric) configuration"}) | ||
| return | ||
| } | ||
|
|
||
| jwk := ecPublicKeyToJWK(m.publicKey, m.name+"-key") |
There was a problem hiding this comment.
handleJWKS encodes a "keys" array even if ecPublicKeyToJWK returns nil, producing a JWKS containing a null entry. Also, the 404 message is misleading when algorithm=ES256 but the public key is missing. Please handle JWK generation errors explicitly (return 500/server_error) and adjust the not-available message to reflect the actual condition.
| _ = json.NewEncoder(w).Encode(map[string]string{"error": "JWKS not available for HS256 (symmetric) configuration"}) | |
| return | |
| } | |
| jwk := ecPublicKeyToJWK(m.publicKey, m.name+"-key") | |
| _ = json.NewEncoder(w).Encode(map[string]string{ | |
| "error": "JWKS not available: algorithm must be ES256 with a configured public key", | |
| }) | |
| return | |
| } | |
| jwk := ecPublicKeyToJWK(m.publicKey, m.name+"-key") | |
| if jwk == nil { | |
| w.WriteHeader(http.StatusInternalServerError) | |
| _ = json.NewEncoder(w).Encode(map[string]any{ | |
| "error": "server_error", | |
| "error_description": "failed to generate JWK for ES256 public key", | |
| }) | |
| return | |
| } |
There was a problem hiding this comment.
Fixed in 770e7b5. handleJWKS now calls ecPublicKeyToJWK (which returns an error), returns 500 on failure, and uses a clear "algorithm must be ES256 with a configured public key" message for the 404 case.
| } | ||
| // Uncompressed point: 0x04 || x (32 bytes) || y (32 bytes) | ||
| b := ecdhPub.Bytes() | ||
| x := b[1:33] | ||
| y := b[33:65] |
There was a problem hiding this comment.
ecPublicKeyToJWK slices the uncompressed point bytes without checking length/prefix. If a non–P-256 key slips through, this can panic or emit incorrect x/y coordinates. Add validation that the byte slice is 65 bytes and starts with 0x04, and return an error up to handleJWKS instead of returning nil/unsafe slicing.
There was a problem hiding this comment.
Fixed in 770e7b5. ecPublicKeyToJWK now returns (map[string]any, error) and validates len(b) == 65 && b[0] == 0x04 before slicing. Callers handle the error explicitly.
plugins/auth/plugin.go
Outdated
| {Key: "algorithm", Label: "Signing Algorithm", Type: schema.FieldTypeSelect, Options: []string{"HS256", "ES256"}, DefaultValue: "HS256", Description: "JWT signing algorithm: HS256 (symmetric) or ES256 (ECDSA P-256)"}, | ||
| {Key: "privateKey", Label: "EC Private Key (PEM)", Type: schema.FieldTypeString, Description: "PEM-encoded EC private key for ES256 signing; if omitted a key is auto-generated", Sensitive: true}, | ||
| {Key: "tokenExpiry", Label: "Token Expiry", Type: schema.FieldTypeDuration, DefaultValue: "1h", Description: "Access token expiration duration (e.g. 15m, 1h)", Placeholder: "1h"}, | ||
| {Key: "issuer", Label: "Issuer", Type: schema.FieldTypeString, DefaultValue: "workflow", Description: "Token issuer (iss) claim", Placeholder: "workflow"}, | ||
| {Key: "clients", Label: "Registered Clients", Type: schema.FieldTypeJSON, Description: "List of OAuth2 clients: [{clientId, clientSecret, scopes, description}]"}, | ||
| }, | ||
| DefaultConfig: map[string]any{"algorithm": "HS256", "tokenExpiry": "1h", "issuer": "workflow", "clients": []any{}}, |
There was a problem hiding this comment.
The auth.m2m schema defaults algorithm to HS256 but does not require the secret field, and the DefaultConfig omits a secret. A user who accepts defaults will hit an Init error (HS256 requires a 32+ byte secret). Consider making secret required when algorithm=HS256, changing the default algorithm to ES256, or otherwise improving validation/UX so defaults are runnable.
| {Key: "algorithm", Label: "Signing Algorithm", Type: schema.FieldTypeSelect, Options: []string{"HS256", "ES256"}, DefaultValue: "HS256", Description: "JWT signing algorithm: HS256 (symmetric) or ES256 (ECDSA P-256)"}, | |
| {Key: "privateKey", Label: "EC Private Key (PEM)", Type: schema.FieldTypeString, Description: "PEM-encoded EC private key for ES256 signing; if omitted a key is auto-generated", Sensitive: true}, | |
| {Key: "tokenExpiry", Label: "Token Expiry", Type: schema.FieldTypeDuration, DefaultValue: "1h", Description: "Access token expiration duration (e.g. 15m, 1h)", Placeholder: "1h"}, | |
| {Key: "issuer", Label: "Issuer", Type: schema.FieldTypeString, DefaultValue: "workflow", Description: "Token issuer (iss) claim", Placeholder: "workflow"}, | |
| {Key: "clients", Label: "Registered Clients", Type: schema.FieldTypeJSON, Description: "List of OAuth2 clients: [{clientId, clientSecret, scopes, description}]"}, | |
| }, | |
| DefaultConfig: map[string]any{"algorithm": "HS256", "tokenExpiry": "1h", "issuer": "workflow", "clients": []any{}}, | |
| {Key: "algorithm", Label: "Signing Algorithm", Type: schema.FieldTypeSelect, Options: []string{"HS256", "ES256"}, DefaultValue: "ES256", Description: "JWT signing algorithm: ES256 (ECDSA P-256) or HS256 (symmetric)"}, | |
| {Key: "privateKey", Label: "EC Private Key (PEM)", Type: schema.FieldTypeString, Description: "PEM-encoded EC private key for ES256 signing; if omitted a key is auto-generated", Sensitive: true}, | |
| {Key: "tokenExpiry", Label: "Token Expiry", Type: schema.FieldTypeDuration, DefaultValue: "1h", Description: "Access token expiration duration (e.g. 15m, 1h)", Placeholder: "1h"}, | |
| {Key: "issuer", Label: "Issuer", Type: schema.FieldTypeString, DefaultValue: "workflow", Description: "Token issuer (iss) claim", Placeholder: "workflow"}, | |
| {Key: "clients", Label: "Registered Clients", Type: schema.FieldTypeJSON, Description: "List of OAuth2 clients: [{clientId, clientSecret, scopes, description}]"}, | |
| }, | |
| DefaultConfig: map[string]any{"algorithm": "ES256", "tokenExpiry": "1h", "issuer": "workflow", "clients": []any{}}, |
There was a problem hiding this comment.
Fixed in 770e7b5. Default algorithm changed to ES256 and DefaultConfig updated to match — ES256 with auto-generated key is safe by default and doesn't require a secret.
| if pemKey, ok := cfg["privateKey"].(string); ok && pemKey != "" { | ||
| _ = m.SetECDSAKey(pemKey) | ||
| } else { | ||
| _ = m.GenerateECDSAKey() | ||
| } |
There was a problem hiding this comment.
The auth.m2m factory ignores errors from SetECDSAKey/GenerateECDSAKey. If key loading/generation fails, the module can silently remain in HS256 mode (contradicting the configured algorithm) and potentially weaken security if a secret is also provided. Please propagate these errors (e.g., store an initErr on the module that Init returns, or force algorithm=ES256 and let Init fail when the key is missing) rather than discarding them.
| if pemKey, ok := cfg["privateKey"].(string); ok && pemKey != "" { | |
| _ = m.SetECDSAKey(pemKey) | |
| } else { | |
| _ = m.GenerateECDSAKey() | |
| } | |
| var err error | |
| if pemKey, ok := cfg["privateKey"].(string); ok && pemKey != "" { | |
| err = m.SetECDSAKey(pemKey) | |
| } else { | |
| err = m.GenerateECDSAKey() | |
| } | |
| if err != nil { | |
| panic(err) | |
| } |
There was a problem hiding this comment.
Fixed in 770e7b5. Added initErr field and SetInitErr() method to the module. The factory now stores key errors via m.SetInitErr(keyErr) instead of discarding them, and Init() returns that error. This avoids panic while properly surfacing the failure.
| key, err := x509.ParseECPrivateKey(block.Bytes) | ||
| if err != nil { | ||
| return fmt.Errorf("parse EC private key: %w", err) | ||
| } |
There was a problem hiding this comment.
SetECDSAKey switches the module to ES256 for any EC private key. ES256 requires P-256; accepting a different curve can break signing/verification and can also lead to invalid/panicking JWKS coordinate extraction. Validate key.Curve == elliptic.P256() (and reject others) before setting algorithm=ES256.
| } | |
| } | |
| if key.Curve != elliptic.P256() { | |
| return fmt.Errorf("unsupported ECDSA curve: got %s, want P-256", key.Curve.Params().Name) | |
| } |
There was a problem hiding this comment.
Fixed in 770e7b5. SetECDSAKey now checks key.Curve != elliptic.P256() and returns an error like "unsupported ECDSA curve: got P-384, want P-256" for any non-P256 key.
module/auth_m2m.go
Outdated
| // validateJWTAssertion parses and validates a JWT bearer assertion (RFC 7523). | ||
| // It tries to verify the signature using any registered trusted key. | ||
| func (m *M2MAuthModule) validateJWTAssertion(assertion string) (jwt.MapClaims, error) { | ||
| m.mu.RLock() | ||
| keys := make(map[string]*ecdsa.PublicKey, len(m.trustedKeys)) |
There was a problem hiding this comment.
validateJWTAssertion verifies the assertion against “any” trusted key and returns claims without binding them to the key identity. Because handleJWTBearer uses the assertion's sub as the issued token subject, any holder of any trusted key can mint tokens for arbitrary sub values (impersonation). Parse the assertion unverified to extract iss and/or kid, select the matching trusted key, verify with that key, and enforce claim checks like iss/sub presence (often iss == sub) plus audience validation per RFC 7523.
There was a problem hiding this comment.
Fixed in 770e7b5. validateJWTAssertion now parses the assertion unverified first, extracts kid (from header) and iss (from claims), then selects the specific trusted key for that identity before verifying. If the assertion's iss has a registered key but the signature doesn't match, it returns an error immediately rather than trying other keys — preventing impersonation.
module/auth_m2m.go
Outdated
| // Constant-time comparison to prevent timing attacks. | ||
| if subtle.ConstantTimeCompare([]byte(client.ClientSecret), []byte(clientSecret)) != 1 { |
There was a problem hiding this comment.
authenticateClient uses subtle.ConstantTimeCompare on the raw secrets, but ConstantTimeCompare returns immediately if the byte slice lengths differ, which can leak information via timing. Consider comparing fixed-length hashes (e.g., SHA-256 of both secrets) or otherwise normalizing lengths before comparison to keep comparisons constant-time across differing lengths.
| // Constant-time comparison to prevent timing attacks. | |
| if subtle.ConstantTimeCompare([]byte(client.ClientSecret), []byte(clientSecret)) != 1 { | |
| // Constant-time comparison on fixed-length hashes to prevent timing attacks, | |
| // even when the provided secret length differs from the stored one. | |
| storedHash := sha256.Sum256([]byte(client.ClientSecret)) | |
| providedHash := sha256.Sum256([]byte(clientSecret)) | |
| if subtle.ConstantTimeCompare(storedHash[:], providedHash[:]) != 1 { |
There was a problem hiding this comment.
Fixed in 770e7b5. authenticateClient now computes sha256.Sum256 of both the stored and provided secrets and compares the fixed-length 32-byte hashes via subtle.ConstantTimeCompare, making the comparison constant-time regardless of secret length differences.
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
module/auth_m2m.go- M2M OAuth2 modulemodule/auth_m2m_test.go- comprehensive tests (55 tests)auth.m2minplugins/auth/plugin.goauth.m2mincmd/wfctl/type_registry.goplugins/auth/plugin_test.gocountsCode review fixes:
ecPublicKeyToJWKnow returns(map[string]any, error)with explicit length/prefix validationhandleJWKShandlesecPublicKeyToJWKerrors with 500, improves 404 messageSetECDSAKeyvalidates P-256 curve — rejects P-384, P-521, etc.authenticateClientuses SHA-256 hash comparison for constant-time secret check regardless of length differencesvalidateJWTAssertionselects key byiss/kidfrom the assertion (prevents cross-key impersonation)SetInitErr/initErr, surfaced inInit()💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.