Skip to content

feature(rp): add support for HS256/HS384/HS512 signatures #719

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mdnight
Copy link

@mdnight mdnight commented Feb 25, 2025

A minor pull request proposed by #412, which introduces support for HS256/HS384/HS512 signatures.

Definition of Ready

  • I am happy with the code
  • Short description of the feature/issue is added in the pr description
  • PR is linked to the corresponding user story
  • Acceptance criteria are met
  • All open todos and follow ups are defined in a new ticket and justified
  • Deviations from the acceptance criteria and design are agreed with the PO and documented.
  • No debug or dead code
  • My code has no repetitions
  • Critical parts are tested automatically
  • Where possible E2E tests are implemented
  • Documentation/examples are up-to-date
  • All non-functional requirements are met
  • Functionality of the acceptance criteria is checked manually on the dev system.

@mdnight mdnight changed the title add HS256/384/512 hash support add HS256/384/512 support Feb 25, 2025
@mdnight mdnight changed the title add HS256/384/512 support add support for HS256/HS384/HS512 signatures Feb 25, 2025
@muhlemmer muhlemmer changed the title add support for HS256/HS384/HS512 signatures feature(rp): add support for HS256/HS384/HS512 signatures Feb 26, 2025
@muhlemmer muhlemmer moved this to 📋 Sprint Backlog in Product Management Feb 26, 2025
@hifabienne hifabienne requested a review from livio-a February 27, 2025 10:33
@hifabienne hifabienne moved this from 📋 Sprint Backlog to 👀 In review in Product Management Mar 3, 2025
@muhlemmer muhlemmer requested review from muhlemmer and removed request for livio-a March 12, 2025 10:51
Copy link
Collaborator

@muhlemmer muhlemmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don;t have anything against support for HS256. However I don't think this implementation is a complete solution.

  1. The linked issue requests functionality for the op package, this PR implements the rp package
  2. Currently the RelyingParty is hard-coded to use a RemoteKeySet from a public jwk_uri endpoint. HS256 being symetrice keys, they cannot be exposed on such a public endpoint. They need to be shared over some other secure channel and stored locally. In order to properly support HS256 in the rp package, loading of local keys must also be made configurable in NewRelyingPartyOIDC. This can be done by implementing a rp.Option

func (rp *relyingParty) IDTokenVerifier() *IDTokenVerifier {
if rp.idTokenVerifier == nil {
rp.idTokenVerifier = NewIDTokenVerifier(rp.issuer, rp.oauthConfig.ClientID, NewRemoteKeySet(rp.httpClient, rp.endpoints.JKWsURL), rp.verifierOpts...)
}
return rp.idTokenVerifier
}

// NewRelyingPartyOIDC creates an (OIDC) RelyingParty with the given
// issuer, clientID, clientSecret, redirectURI, scopes and possible configOptions
// it will run discovery on the provided issuer and use the found endpoints
func NewRelyingPartyOIDC(ctx context.Context, issuer, clientID, clientSecret, redirectURI string, scopes []string, options ...Option) (RelyingParty, error) {
rp := &relyingParty{
issuer: issuer,
oauthConfig: &oauth2.Config{
ClientID: clientID,
ClientSecret: clientSecret,
RedirectURL: redirectURI,
Scopes: scopes,
},
httpClient: httphelper.DefaultHTTPClient,
oauth2Only: false,
oauthAuthStyle: oauth2.AuthStyleAutoDetect,
}
for _, optFunc := range options {
if err := optFunc(rp); err != nil {
return nil, err
}
}
ctx = logCtxWithRPData(ctx, rp, "function", "NewRelyingPartyOIDC")
discoveryConfiguration, err := client.Discover(ctx, rp.issuer, rp.httpClient, rp.DiscoveryEndpoint)
if err != nil {
return nil, err
}
if rp.useSigningAlgsFromDiscovery {
rp.verifierOpts = append(rp.verifierOpts, WithSupportedSigningAlgorithms(discoveryConfiguration.IDTokenSigningAlgValuesSupported...))
}
endpoints := GetEndpoints(discoveryConfiguration)
rp.oauthConfig.Endpoint = endpoints.Endpoint
rp.endpoints = endpoints
rp.oauthConfig.Endpoint.AuthStyle = rp.oauthAuthStyle
rp.endpoints.Endpoint.AuthStyle = rp.oauthAuthStyle
// avoid races by calling these early
_ = rp.IDTokenVerifier() // sets idTokenVerifier
_ = rp.ErrorHandler() // sets errorHandler
_ = rp.UnauthorizedHandler() // sets unauthorizedHandler
return rp, nil
}

@@ -186,7 +186,7 @@ func toJoseSignatureAlgorithms(algorithms []string) []jose.SignatureAlgorithm {
out[i] = jose.SignatureAlgorithm(algorithms[i])
}
if len(out) == 0 {
out = append(out, jose.RS256, jose.ES256, jose.PS256)
out = append(out, jose.RS256, jose.ES256, jose.PS256, jose.RS256)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose you meant to add HS256 here?

However, let's not add HS256 as a default, as it also influences the op package. Your PR only implemented the rp part. HSxxx can still be configured when creating the the Verifier in the RP.

@muhlemmer muhlemmer added the waiting For some reason, this issue will have to wait. This can be a feedback that is being waited for, a de label Mar 12, 2025
@muhlemmer
Copy link
Collaborator

@mdnight any feedback on the made comments?

@mdnight
Copy link
Author

mdnight commented Mar 29, 2025

Hey, sorry for the late response. I am in the process of implementing the missing stuff that you mentioned in the comments. It may take some time because I am working on it in my spare time, but I will try to get it done anyway. Hopefully, this is a low-priority feature and there are no strict deadlines on this 😄

@mdnight mdnight marked this pull request as draft March 30, 2025 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os-contribution waiting For some reason, this issue will have to wait. This can be a feedback that is being waited for, a de
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

3 participants