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.

Sign up for GitHub

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

Jump to bottom

Encrypt OIDC cookies #12628

Merged
merged 12 commits into from
Feb 6, 2024
Merged

Encrypt OIDC cookies #12628

merged 12 commits into from
Feb 6, 2024

Conversation

markylaing
Copy link
Contributor

@markylaing markylaing commented Dec 6, 2023 •
edited
Loading

This PR encrypts the OIDC cookies that are set in the response after logging in to LXD. It finishes the implementation of changes to handling of OIDC authentication as discussed in #12531.

To ensure that the encryption key is the same on all cluster members, we are deriving the key from the cluster private key using HKDF.

Closes #12531

@markylaing markylaing self-assigned this Dec 6, 2023
This was referenced Dec 6, 2023
Closed
Closed
gabrielmougard
gabrielmougard reviewed Dec 6, 2023
View reviewed changes
Copy link
Contributor

@gabrielmougard gabrielmougard left a comment

Choose a reason for hiding this comment

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

A couple of comments but it looks good

lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Dec 7, 2023
View reviewed changes
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Dec 7, 2023
View reviewed changes
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Dec 7, 2023
View reviewed changes
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Dec 7, 2023
View reviewed changes
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
nsklikas
nsklikas reviewed Dec 7, 2023
View reviewed changes
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
roosterfish
roosterfish reviewed Dec 8, 2023
View reviewed changes
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Hi, looks very good overall. Just a few smaller suggestions.

lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
lxd/auth/oidc/oidc.go Show resolved Hide resolved
@markylaing markylaing force-pushed the oidc-fixes branch 2 times, most recently from 994e007 to 76a2445 Compare December 12, 2023 10:50
@markylaing
Copy link
Contributor Author

@nsklikas In the OIDC login handler I have re-added the audience parameter. While it may not form part of the full OIDC specification, it is required if we want to add custom claims to tokens (e.g. group membership) which we will need.

roosterfish
roosterfish previously approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Thanks for the clarifications, LGTM.

tomponline
tomponline reviewed Dec 12, 2023
View reviewed changes
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Feb 5, 2024
View reviewed changes
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Feb 5, 2024
View reviewed changes
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
tobhe
tobhe approved these changes Feb 5, 2024
View reviewed changes
Copy link
Member

@tobhe tobhe left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me now. I think you addressed all of the changes we requested:

  • You are using salt and context information for the keys
  • sessionId (the salt) is regenerated for each token refresh
  • salt size of 64 is also sufficient

I agree with @tomponline that a few places could benefit from some more documentation.

chrisccoulson
chrisccoulson reviewed Feb 5, 2024
View reviewed changes
Copy link

@chrisccoulson chrisccoulson left a comment

Choose a reason for hiding this comment

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

Thanks, I've left a couple of minor comments

lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved
@markylaing
lxd/auth/oidc: Add constants for session cookie and config refresh in…
a87fdcf 
…terval.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/oidc: Add fields for secure cookie handling.
e76aca7 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/oidc: Initialise Verifier with fields for cookie encryption.
177bad4 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
tomponline
tomponline previously approved these changes Feb 6, 2024
View reviewed changes
@markylaing
lxd/auth/oidc: Add method to get securecookie from session ID.
e183169 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/oidc: Updates methods for getting and setting cookies.
0695289 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/oidc: Updates calls to get or set cookies.
87e9296 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/oidc: Rotate the encryption key of the relying party.
db3433c 
These cookies do not need to be decrypted by other cluster members,
so it is ok to use secure key generation that is built-in to the
securecookie library. This also reduces the exposure of our private key.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Update calls to NewVerifier.
8311ab9 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/oidc: Request is not OIDC if session cookie not present.
fa47a23 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Fix lint error (revive: confusing-results).
e0a4404 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Fix lint errors (var-naming).
bb44108 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Fix lint errors (revive: unchecked-type-assertion).
8d4b2e4 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
tomponline
tomponline approved these changes Feb 6, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Thanks!

@tomponline tomponline merged commit 5e6202c into canonical:main Feb 6, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

@shipperizer shipperizer Awaiting requested review from shipperizer

@MusicDin MusicDin Awaiting requested review from MusicDin

@massigori massigori Awaiting requested review from massigori

@simondeziel simondeziel Awaiting requested review from simondeziel

@nsklikas nsklikas Awaiting requested review from nsklikas

@gabrielmougard gabrielmougard Awaiting requested review from gabrielmougard

@roosterfish roosterfish Awaiting requested review from roosterfish

@chrisccoulson chrisccoulson Awaiting requested review from chrisccoulson

@tobhe tobhe Awaiting requested review from tobhe

Assignees

@markylaing markylaing

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OIDC implementation discussion
8 participants
@markylaing @tomponline @tobhe @chrisccoulson @simondeziel @roosterfish @nsklikas @gabrielmougard