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

feat: SSO through OpenID Connect #609

Merged
merged 17 commits into from
Mar 27, 2023
Merged

feat: SSO through OpenID Connect #609

merged 17 commits into from
Mar 27, 2023

Conversation

oleobal
Copy link
Contributor

@oleobal oleobal commented Mar 7, 2023

Description

This adds an OpenID Connect client to the Substra backend. All negotiation with the identity provider is handled by the backend.

We use the mozilla-django-oidc library, but covering for its limitations and fitting in well with the backend ultimately means writing a lot of code, unfortunately. It would be possible to roll some back into the lib, but not very much.

New stuff

API extension

It adds endpoints under /oidc. The authentication endpoint ultimately sets the JWT cookies, just like /me/login, so the frontend doesn't suspect a thing.

It adds a new /api-auth-tap endpoint (I'm open to feedback on the name), which works like /api-auth-token, but rather than POST your creds to it you need to be already authenticated somehow to access it. This is so SSO users can get bearer tokens to use in the Python code. Some day down the line we will have something more fully-featured than just returning always the same token.

It adds a new field in the dictionary returned by /info: auth, which contains info about available authentication methods. This is intended for use by the frontend (so it can add a new button to the login page).

In the /users endpoint of the API, there is a new is_external_user boolean field, which the frontend may use to disable some functions (like changing the password).

User model extension

The user model is expanded with a UserOidcInfo, which contains an openid_subject (the identifier given to us by the provider, which is unique and unchanging) alongside an openid_issuer (the identity of the provider -- should be the same for all OIDC users, but will save us headaches if we ever support more than one).

To cite the spec

The sub (subject) and iss (issuer) Claims, used together, are the only Claims that an RP can rely upon as a stable identifier for the End-User, since the sub Claim MUST be locally unique and never reassigned within the Issuer for a particular End-User, as described in Section 2.2. Therefore, the only guaranteed unique identifier for a given End-User is the combination of the iss Claim and the sub Claim.
(source)

By default mozilla-django-oidc filters on email alone but I'm uneasy about this.

UserOidcInfo also contains a valid_until field, which is used to restrict access of associated API tokens: the environment maintainer can choose how long a given user can use tokens associated with their account before they need to OIDC-login again (to validate they still have an account at the identity provider).

Because this is a pain, if possible Substra will request a refresh token, so that it can check the user is still valid in the background. However not all identity providers implement that, and not all in the same way, so there is the manual fallback still.

Notes, questions and limitation

A significant limitation is that we only handle a single provider. The library we use is set up this way, we could work on it in the future to get rid of this limitation though. Also, although a PR to handle PKCE (increase security) has been merged there, it hasn't been released yet.

The other thing is that I end up replacing a lot of the library code -- the point is never to override security-sensitive code (so security isn't managed by us), but it has downsides:

  • a lot of this code is basically unreadable unless you're working off the library code as reference
  • we're doing something the library wasn't meant to do : request and store a refresh token alongside the user info, which requires an ugly hack

How has this been tested?

Through local testing

This uses node-oidc-provider.

package.json:

{
  "dependencies": {
    "oidc-provider": "^8.1.0"
  },
  "main": "src/server.js",
  "type": "module"
}

server.js:

import Provider from 'oidc-provider';
const configuration = {
  clients: [{
  {
    client_id: 'substra_backend',
    client_secret: 'somesecret',
    redirect_uris: [
      'http://substra-backend.org-1.com/oidc/callback/',
      'http://substra-backend.org-2.com/oidc/callback/',
    ],
    grant_types: ['authorization_code', 'refresh_token'],
    scope: 'openid offline_access email',
    //response_types: ['code'], 
  }
  ],
  pkce: {
    required: function pkceRequired(ctx, client) {
      return false;
    }
    // not yet implemented (https://github.com/mozilla/mozilla-django-oidc/pull/474
  },
  claims: {
    email: ['email', 'email_verified', 'name']
  },
  async findAccount(ctx, id) {
    return {
      accountId: id,
      async claims(use, scope) { return { sub: id, email: "[email protected]", name: "Olivier Léo" }; },
    };
  }
};



const oidc = new Provider('http://localhost:4000', configuration);

const server = oidc.listen(4000, () => {
  console.log('oidc-provider listening on port 4000, check http://localhost:4000/.well-known/openid-configuration');
});

Run it: node src/server.js

Edit your /etc/hosts so oidc-provider points to localhost, and also kubectl apply -n org-1 -f this on the server:

kind: Service
apiVersion: v1
metadata:
 name: oidc-provider
spec:
 type: ExternalName
 externalName: host.k3d.internal

That way oidc-provider points to the same thing whether it's on your browser or inside the cluster (which is required for these shenanigans).

The Skaffold configuration includes OIDC. Deploy, visit /oidc/authenticate, and enjoy. In dev mode, it logs you in via Django session so you can use the DRF API browser -- in prod mode, it just gives you the JWT cookies.

After logging in, you can get a bearer token at /api-token-tap/ and use it in Python substra:

from substra import Client
client = Client("http://substra-backend.org-1.com", "<token>")
print(client.organization_info())

And also through Google! Turns out Google doesn't obey the OpenID spec regarding refresh tokens, fun stuff. But it works!

Checklist

  • created users are added to a default channel
  • the authentication endpoint can redirect to where it is best
  • user management takes into account external users, which have no password and cause the end of the world when they change their email
    • there should probably be a valid_until field in UserOidcInfo that tracks how long the issued SSO token was valid for.
    • Substra-issued access token track SSO tokens somewhat
  • there is an endpoint to create Bearer tokens for the substra lib
  • changelog was updated with notable changes
  • documentation was updated

This PR should be followed or accompanied by:

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 7, 2023
@oleobal oleobal force-pushed the feat/sso-oidc branch 6 times, most recently from 9d02223 to cc85041 Compare March 11, 2023 15:42
@oleobal oleobal force-pushed the feat/sso-oidc branch 8 times, most recently from e2fba99 to 2854f19 Compare March 16, 2023 15:37
@@ -23,6 +23,7 @@ django-prometheus==2.2.0
django-filter==22.1
pydantic==1.10.1
redis==4.3.4
mozilla-django-oidc==3.0.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presumably 3.1.0 will add support for PKCE (it's in master but hasn't been released)

@@ -1,5 +1,4 @@
.PHONY: doc
doc:
# There is no release of the readme-generator-for-helm tool on the repo so we target an arbitrary commit
npx https://github.com/bitnami-labs/readme-generator-for-helm/tree/3300343a6cd1c9cd86d13b04d8c85a7415cb849e -v substra-backend/values.yaml -r substra-backend/README.md
npx https://github.com/bitnami-labs/readme-generator-for-helm/tree/2.5.0 -v substra-backend/values.yaml -r substra-backend/README.md
Copy link
Contributor Author

@oleobal oleobal Mar 16, 2023

Choose a reason for hiding this comment

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

2.5.0 adds a useful @descriptionStart feature

@oleobal oleobal added python Pull requests that update Python code api labels Mar 16, 2023
@oleobal oleobal marked this pull request as ready for review March 16, 2023 16:30
@oleobal oleobal requested a review from a team as a code owner March 16, 2023 16:30
@github-actions github-actions bot removed the api label Mar 16, 2023
Copy link
Contributor

@guilhem-barthes guilhem-barthes 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 your contribution! Overall a nice feature on its way, although I made some comments about refactoring and optimizing some DB requests.

backend/backend/settings/deps/oidc.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
backend/backend/settings/deps/oidc.py Outdated Show resolved Hide resolved
backend/backend/settings/deps/oidc.py Outdated Show resolved Hide resolved
backend/backend/settings/localdev.py Show resolved Hide resolved
backend/users/authentication.py Show resolved Hide resolved
backend/users/utils.py Outdated Show resolved Hide resolved
backend/users/utils.py Outdated Show resolved Hide resolved
backend/users/utils.py Outdated Show resolved Hide resolved
backend/users/utils.py Outdated Show resolved Hide resolved
@oleobal oleobal force-pushed the feat/sso-oidc branch 3 times, most recently from 1ccfeeb to 548c430 Compare March 22, 2023 16:34
@oleobal oleobal force-pushed the feat/sso-oidc branch 4 times, most recently from a3982fe to e6aa275 Compare March 24, 2023 10:12
Copy link
Contributor

@guilhem-barthes guilhem-barthes left a comment

Choose a reason for hiding this comment

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

LGTM

oleobal added 16 commits March 25, 2023 02:10
Signed-off-by: Olivier Léobal <[email protected]>
Signed-off-by: Olivier Léobal <[email protected]>
Signed-off-by: Olivier Léobal <[email protected]>
Signed-off-by: Olivier Léobal <[email protected]>
Signed-off-by: Olivier Léobal <[email protected]>
Not needed right now but will save us headaches if we ever support more than one issuer

Signed-off-by: Olivier Léobal <[email protected]>
Signed-off-by: Olivier Léobal <[email protected]>
Signed-off-by: Olivier Léobal <[email protected]>
Signed-off-by: Olivier Léobal <[email protected]>
Signed-off-by: Olivier Léobal <[email protected]>
Signed-off-by: Olivier Léobal <[email protected]>
@oleobal oleobal merged commit 9ec39e6 into main Mar 27, 2023
@oleobal oleobal deleted the feat/sso-oidc branch March 27, 2023 07:53
@oleobal oleobal mentioned this pull request Mar 27, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants