Skip to content

Conversation

@benschweizer
Copy link
Contributor

related code changes in influxdata/chronograf#2526

@stevebang stevebang requested review from jaredscheib and removed request for nhaugo March 13, 2018 15:05
@stevebang
Copy link
Contributor

@jaredscheib Please review the content here related to Generic auth, and we can merge the changes to our recent changes.

Copy link
Contributor

@jaredscheib jaredscheib 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 submitting documentation with your feature changeset, @CXCV -- we really appreciate it. I've made a number of requests below in regards to organization of ideas and syntax.

I haven't tested this as I don't have an ADFS sandbox, so I'm just going to take it on faith that what you've written here is accurate and conceptually comprehensive :)

```

**JWKS Signature Verification**
If the provider implements OpenID Connect with RS256 signatures (as Microsoft ADFS does), you need to provide a JWKS document (holding the ca certificates) so we can validate the RSA signatures against. These certificates are regularly rolled over, so we'll fetch them from the JWKS_URL on demand.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. what is a ca certificate? i don't see it mentioned at https://tools.ietf.org/html/rfc7517#section-4.7
  2. so we can validate the RSA signatures against, as long as i understand the concept correctly, would more grammatically be replaced by , to validate the RSA signatures against. or against which the RSA signatures can be validated.
  3. can you put in parenthesis after rolled over something like (expired) or something else that clarifies what "rolled over" means?
  4. could you add the concept of id_token parsing here somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CA = Certificate Authority; I'll replace it with "certificate chain" which is used in the RFC.

#### Examples
##### OpenID Connect (OIDC) / Active Directory Federation Services (ADFS)

See [Enabling OpenId Connect with AD FS 2016](https://docs.microsoft.com/en-us/windows-server/identity/ad-fs/development/enabling-openid-connect-with-ad-fs) for a walk through of the server configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. OpenId --> OpenID
  2. could you either change AD FS --> ADFS or otherwise change all of the other instances to AD FS for consistency?


Also, on the Chronograf login page, the text on the authentication button changes from `Log in with generic` to `Log in with GitLab`.

#### Examples
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm fine with introducing an Examples section, but that's up to @stevebang. if we do, @stevebang, it may be good to add examples for the others in a separate PR.


See [Enabling OpenId Connect with AD FS 2016](https://docs.microsoft.com/en-us/windows-server/identity/ad-fs/development/enabling-openid-connect-with-ad-fs) for a walk through of the server configuration.

Exports for chronograf (e.g. in /etc/default.chronograf):
Copy link
Contributor

Choose a reason for hiding this comment

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

chronograf --> Chronograf


The generic OAuth 2.0 provider is very similar to the GitHub provider, but
you are able to set your own authentication, token, and API URLs.
Additionally, this provider implements OpenID Connect (OIDC) id_token parsing, as implemented by Active Directory Federation Services (ADFS).
Copy link
Contributor

@jaredscheib jaredscheib Mar 13, 2018

Choose a reason for hiding this comment

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

i'm not sure if this comment is necessary, since each export is required by different services and we don't direct attention to any other specific ones here. a user would still be able to see that ADFS is supported in the JWKS Signature Verification section. the only thing to keep is the concept of id_token parsing, which i have proposed moving to that section. could you move id_token parsing there and remove this comment?

Exports for chronograf (e.g. in /etc/default.chronograf):
```sh
PUBLIC_URL="https://example.com:8888"
GENERIC_CLIENT_ID="chronograf"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a likely client id, or would we be more likely to see an arbitrary string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For AD FS, you need to keep it simple. AD FS is splitting this string on colons, resulting in server-side failures. Probably the use a url-parser here, but I think this is not required by the specs. I've added a info to the docs.

@monitoringit
Copy link

I am trying to make ADFS 4.0 work with chronograf oauth2. Does it mean that currently chronograf does not support oauth2 under Generic identity provider for ADFS 4.0?

@jaredscheib
Copy link
Contributor

jaredscheib commented Mar 26, 2018

Hi @monitoringit, I believe once the work from influxdata/chronograf#2526 is merged into Chronograf (pending resolution of merge conflicts), Chronograf will support ADFS 4.0 as a Generic OAuth2 identity provider.

@monitoringit
Copy link

Jared...may be my understanding is wrong about oauth2 ....but as i understand this PR is about using openid and oauth2 together...can we still purely use oauth2 with ADFS without using any openid reference?

@benschweizer
Copy link
Contributor Author

@monitoringit I've made the OpenID id_token handling optional, see USE_ID_TOKEN option

@monitoringit
Copy link

monitoringit commented Mar 28, 2018

cxcv, So is the inference here is that i should be able to use chronograf oauth2 with ADFS 4 if i dont use open id references. I am using chronograf v1.4.0.0 with below settings and i am having challenges making it work. Do you have any experience making it work with using only ADFS oauth2?
...
...
BASE_PATH=/chronograf
PREFIX_ROUTES=true
REPORTING_DISABLED=true
GENERIC_CLIENT_ID=xxxxxxxxxxxxxxxxxxxxxx
GENERIC_CLIENT_SECRET=xxxxxxxxxxxxxxxxxxxxxxx
AUTH_DURATION=1h
GENERIC_SCOPES=email
TOKEN_SECRET=xxxxxx
PUBLIC_URL=https://chronoserver.com/chronograf
GENERIC_AUTH_URL=https://adfsserver./adfs/oauth2/authorize
GENERIC_TOKEN_URL=https://adfsserver./adfs/oauth2/token
...
...
Call back url set on adfs server for chronograf app is ==>https://chronoserver.com/chronograf/chronograf/oauth/generic/callback [used chronograf twice here because of prefix route set to chronograf].

@timhallinflux
Copy link
Contributor

@monitoringit as @jaredscheib mentions -- ADFS 4 isn't going to work with Chronograf v1.4.0.0. Once the work from influxdata/chronograf#2526 is merged into Chronograf, Chronograf will support ADFS 4.0 as a Generic OAuth2 identity provider. I believe this is planned for 1.4.3.0...and we are doing final testing on this week. Stay tuned for the release! Please let us know once you get a chance to try it out.

@timhallinflux timhallinflux added this to the 1.4.3 milestone Mar 28, 2018
@monitoringit
Copy link

Thanks timhall for clarification. Will checkout 1.4.3 once released.

@timhallinflux
Copy link
Contributor

@monitoringit -- I made a small mistake here. We have some generic OAuth fixes in 1.4.3 -- but that will NOT be sufficient for the ADFS 4 config. We do have that fix merged into master now... but that would mean you would have to build from source and give it a shot. We are looking at generating nightlies at the moment. I'm curious if you are just downloading bits from our website...building yourself...or ??

@timhallinflux timhallinflux modified the milestones: 1.4.3, 1.5 Mar 28, 2018
@jaredscheib
Copy link
Contributor

@monitoringit To further clarify: this ADFS feature is now currently in the Chronograf master branch, but it is not included in the 1.4.3.0 release going out today. It is currently scheduled for release in 1.5.0.0, which is our next release and should be published in about 2 weeks. If you want to try out ADFS now, you can build from Chronograf source on the master branch as of now. If that's not feasible, we are looking into building nightlies for Chronograf but don't currently have a process in place for that.

@jaredscheib
Copy link
Contributor

jaredscheib commented Mar 28, 2018

And note, @CXCV and all: this docs PR will be merged once Chronograf 1.5.0.0 (or the next version) is ready for release, which is currently scheduled for about 2 weeks from now.

@monitoringit
Copy link

Thank you! Can you share ur chronograf adfs configuration ( set of env vars with values)?...i tried with chrono config as shared above in the this thread...

@jaredscheib
Copy link
Contributor

@monitoringit I believe the information you're looking for is available in the content of this PR, which can be found at https://github.com/cxcv/docs.influxdata.com/blob/8d7f5d0ba50d0e85da589e096c9e3698f8989229/content/chronograf/v1.4/administration/managing-security.md#openid-connect-oidc--active-directory-federation-services-ad-fs. Try that and let us know how it goes. Thanks!

@monitoringit
Copy link

Thanks!! Will try it out once 1.5 is released

@benschweizer
Copy link
Contributor Author

@monitoringit As @jaredscheib writes, 1.5 will bring OpenID support for AD FS, and this PR includes a example section with all the environment variables. This will be the right thing to do.

It is possible (but discouraged) to authenticate against AD FS without OpenID, but you won't have username, email address or groups available then. The trick there is, to set the GENERIC_API_KEY to the only available data: "sub". This results in your username? being some random string, which is intended to be an random identifier, not your username. The env for this not recommended auth is like:

PUBLIC_URL=https://chronograf.example.com:8888
GENERIC_CLIENT_ID=chronograf
GENERIC_CLIENT_SECRET=dsdsdslkajflkdasjflkasjflkasjflasjdflk
GENERIC_AUTH_URL="https://ad.example.com/adfs/oauth2/authorize"
GENERIC_TOKEN_URL="https://ad.example.com/adfs/oauth2/token"
GENERIC_SCOPES="openid email"
TOKEN_SECRET=dasdsasdfasdjflkasdjfölkasjf
GENERIC_API_KEY=sub
GENERIC_API_URL=https://ad.example.com/adfs/userinfo
GENERIC_NAME="generic"

The problem here is, that there's no userinfo available. The userinfo is included in an extra "id_token", which requires some extra processing. And that extra processing is exactly what this patch implements.

Cheers
Benjamin

@sanderson
Copy link
Contributor

This functionality was included in Chronograf 1.4.4 and is currently available.

@sanderson sanderson merged commit 6ac295e into influxdata:master Apr 18, 2018
@jaredscheib
Copy link
Contributor

@monitoringit As per what @sanderson said above, this functionality is now live in Chronograf 1.4.4.x. I'd be interested to hear your experience with it!

@monitoringit
Copy link

Will try it out and share my experience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants