Skip to content

Conversation

@daikema
Copy link
Contributor

@daikema daikema commented Sep 19, 2022

Due to some identity providers not supporting public clients, the decision was made to reimplement / supersede #119 but with the bulk of the changes handled on the serverside to avoid the need to use public clients. Making the changes in this way also significantly reduced the need for changes to the carta-frontend codebase. Tokens are issued similar to when using LDAP or PAM auth, but with additional state information stored in client cookies encrypted with a symmetric key.

Test was done with both Keycloak and Google as identity provider. It should be noted using Google as an identity providers requires some additional parameters be passed if refresh tokens are desired, and this can be handled by passing in the parameters "additionalAuthParams": [["access_type", "offline"], ["prompt", "consent"]] in the OIDC provider section of the config.

(Note that while the existing local config uses the jsonwebtoken package, this pull request instead uses jose due to the latter's support for encryption in addition to signing ability as well as the latter package's support for JWKS)

@daikema
Copy link
Contributor Author

daikema commented Sep 23, 2022

One thing that I did notice when experimenting with Google account linking to Keycloak that you might get mapped to a non-existent posix account. (Note that this approach I've been testing is not the same as logging in through Google as identity provider - this is using keycloak as identity provider but logging into Keycloak via a Google account).

So out of the box instead of getting mapped to my username account, I'd instead end up with an auto-created keycloak account (username@idia.ac.za being how it was getting mapped) which doesn't have a corresponding local POSIX account. At the moment you can login to the dashboard with that, but there's no testing for the existence of a matching posix account until an attempt is made to actually launch a CARTA process.

@veggiesaurus guessing that you'd like me to verify an existing posix account with the mapped username before getting to the dashboard portion of the process?

@daikema
Copy link
Contributor Author

daikema commented Sep 26, 2022

Will note that if we do want to verify that the posix user exists this may be slightly more complicated than expected.

node doesn't seem to have any direct way of calling out to functions like getpwnam to retrieve the account information. That function is accessible if you install posix but that package has an unpatched high-severity vulnerability reported by npm audit and unfixed since discovery in June. Note that shortly after the issue was discovered a pull request in the posix repo was opened which claims to fix the issue so it appears the issue is that posix maintainer has disappeared.

Other packages that allow you to access user records like whomst will pull in that posix module as a dependency as default to using it.

@veggiesaurus veggiesaurus self-assigned this Sep 28, 2022
]
},
"symmetricKeyType": {
"description": "As per options at https://www.iana.org/assignments/jose/jose.xhtml when using direct use of shared key",
Copy link
Collaborator

Choose a reason for hiding this comment

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

"when using direct use of shared key" sounds odd. Should this be "when a shared key is used directly"?

The description also doesn't describe the option. Should this be "Type of symmetric key as per (...)"

Is the "shared key" the same thing as the "symmetric key"? Is the "shared key" qualification also needed for the location option above?

@daikema daikema marked this pull request as draft November 22, 2022 11:12
@daikema daikema marked this pull request as ready for review March 19, 2023 12:03
@daikema
Copy link
Contributor Author

daikema commented Mar 19, 2023

So I've marked this as ready for review after some discussion since, as per this issue we have a looming deadline on the Google front combined with upcoming temporary unavailability of some people.

As per that discussion, it's likely that some additional dev work will be done in separate pull requests re:
(1) Further documentation changes (I've made some modifications, but perhaps not enough to make @confluence happy yet), but the plan is to separate that out somewhat.
(2) Making the crypto bits more flexible with less hard-coding (i.e. at present the crypto algorithms in use in places is hardcoded along with key and IV sizes).

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.

4 participants