Skip to content
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

Added settings to reduce tokensize for OpenId #4129

Conversation

MatthijsKrempel
Copy link
Contributor

For hosting in cloud based environments header size matters. Currently the default implementation of the OpenId serializes a lot of information into the security ticket, resulting into large headersizes of the request. With these three extra options the administrator will be able to control what claims are included into the ticket, reducing the size of the values in the header. Additional information that a service would require could be retrieved via an additional request to the OpenId server.

@MatthijsKrempel
Copy link
Contributor Author

@PinpointTownes can you have a look?

@kevinchalet
Copy link
Member

Sounds like a good start, but shouldn't we go further and introduce a mapping feature that allows listing all the claims we want in access and identity tokens, possibly with a different name?

I'm pretty sure @sebastienros and I chatted about that last week. He certainly has some input about that feature.

@MatthijsKrempel
Copy link
Contributor Author

Could you draw me a picture of what you would like the end result to look like? I would be happy to help.
For now, our production clusters are struggeling with the large tokensizes in headers. Our ingress and kestrel are throwing errors, so this is sort of a 'quick fix'.

@MatthijsKrempel
Copy link
Contributor Author

@PinpointTownes do you have any idea what the end result would look like?

@kevinchalet
Copy link
Member

It could be as simple as 2 textboxes and 2 checkboxes: one for the input claim name, one for the output claim name and the 2 checkboxes for the claim destination (access and identity token).

Or it could be more elaborate and use some scripting engine. I believe @MichaelPetrinolis worked on something similar to allow mapping external claims. @MichaelPetrinolis do you confirm? 😄

@MichaelPetrinolis
Copy link
Contributor

I confirm what @PinpointTownes says, you could use IScriptingManager to filter the claims that will be included in the ticket. Providing a default script and adequate documentation/examples, that would be an easy and powerful feature. Check PR #3722 for a screenshot and how mapping external claims to local roles is done.

1 checkbox and 1 textbox 😄

@MatthijsKrempel
Copy link
Contributor Author

@PinpointTownes if I pinkyswear that I will work on this mapper, can we put this in, in the mean time?

@kevinchalet
Copy link
Member

That's a question for @sebastienros, as I'm not sure what the current roadmap looks like.

If we "temporarily" merge this change, we'll need to replace it quickly to make sure it's not part of a public release (otherwise, it will be hard to remove it in a future version).

@agriffard
Copy link
Member

@MatthijsKrempel Please conflicts to solve also here.

@agriffard
Copy link
Member

@MatthijsKrempel Please merge dev and solve the conflicts.

@Piedone
Copy link
Member

Piedone commented Jan 11, 2024

I sincerely apologize for us taking so much time here. I checked out the PR, going over old ones, and this would be quite useful.

Is this something you'd like to revisit @MatthijsKrempel or should we close? @kevinchalet with a review?

@MatthijsKrempel
Copy link
Contributor Author

MatthijsKrempel commented Jan 11, 2024 via email

@Piedone
Copy link
Member

Piedone commented Jan 11, 2024

Thanks for your quick reply! I've opened #15067 to keep track of this, should anybody be interested to take over.

@Piedone Piedone closed this Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants