-
Notifications
You must be signed in to change notification settings - Fork 473
Description
Background
This issue tracks a refactoring of samlsp based on the types of issues and PRs we've been getting. The intention is to make the package a bit more modular to support appropriate customization without needing to fork etc.
Some examples:
- In Obtain SessionIndex in JWT token? #219, @hielfx needs the session index
- In Token cookie can exceed 4kb #203, @BryceDFisher needs to compress the session cookie, because it is too big.
- In Including "jti" (JWT ID) Claim #175, @asteriosgr wants to modify the claims made in the session cookie JWT
- In PR samlsp: move the setting and reading of cookies into an interface #133, I (@crewjam) introduced an abstraction to abstract how we handle cookies, but it was insufficiently abstract to address the other issues.
In the current design, there aren't a lot of good ways to extend/customize the middleware.
Proposal
Split the Middleware component into three interfaces (in addition to the saml.ServiceProvider:
RequestTracker(better name needed?) which will handle tracking pending authn requestsSessionwhich will handle issuing session cookiesOnErrorwhich handles reporting errors to both the user and any logging.
The default implementations of RequestTracker and Session use http cookie to encode JWTs, but further delegate the encoding/verification of the JWTs to codec interfaces which allow further customization, again with default implementations.
My work in progress branch is refactor_samlsp (PR #230)
Compatibility
This will require changes to the public interfaces, which although "allowed" by semver because we are < 1.0, might still be annoying. Tracking some usages of samlsp to see how/if they will be affected:
-
Rancher Provides a custom cookie store, I think only for the purpose of specifying the cookie domain. (@mrajashree)
-
Subspace re-implements the SAML metadata fetching (now in ParseMetadata) and instantiates samlsp.New() with Options. This usage will be simplified by these changes, but looks like it won't break.
-
At grafana, @gotjosh and crew are using samlsp with Keycloak, although I'm not exactly sure how. They may wish to comment on if/how this change would affect them. (ref Certificate validation for RSA is optional? #225)
-
In saml-proxy @dustin-decker is using samlsp.New(), and it looks like our change won't break their usage.
-
In ecs-deploy @wardviaene is doing a similar thing, although they seem to have had to copy & paste quite a bit from samlsp, which perhaps could have been avoided if OnError existed.
-
vili also looks like it won't break.