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

Support used self-signed certificates LDAP. #1278

Merged
merged 1 commit into from
Sep 22, 2018
Merged

Support used self-signed certificates LDAP. #1278

merged 1 commit into from
Sep 22, 2018

Conversation

liweiv
Copy link
Contributor

@liweiv liweiv commented Aug 26, 2018

support used self-signed certificates LDAP. #1277

srenatus
srenatus reviewed Sep 6, 2018
View reviewed changes
connector/ldap/ldap.go Outdated
@@ -68,7 +68,10 @@ type Config struct {

// Path to a trusted root certificate file.
RootCA string `json:"rootCA"`

// Path to a self-signed certificates ca file.
ClientCA string `json:"clientCA"`
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Wouldn't this mostly match the RootCA? I'm not sure I understand the usecase where it differs. Can you please elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If ldap server setting verify client cert is true, only setting RootCA cannot work.

connector/ldap/ldap.go Outdated

// Path to a self-signed certificates ca file.
ClientCA string `json:"clientCA"`
//Path to a self-signed certificates private key file.
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Can we make that // Path (with a space)?

@srenatus
Copy link
Contributor

srenatus commented Sep 6, 2018

@veily Thanks for your contribution 😃

srenatus
srenatus reviewed Sep 7, 2018
View reviewed changes
Copy link
Contributor

@srenatus srenatus 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 renaming that, I was quite confused 😉

connector/ldap/ldap.go Outdated
@@ -68,7 +68,10 @@ type Config struct {

// Path to a trusted root certificate file.
RootCA string `json:"rootCA"`

// Path to a self-signed certificates cert file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the "self-signed" there? I don't think it is necessary to have self-signed cert here.

Copy link
Contributor Author

@liweiv liweiv Sep 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I am not clear enough. The self-signed certificate is not dex. The ldap server uses a self-signed certificate (RootCA, server cert) and opens the certificate for the client. In this case, if dex Ability to authenticate client certificates through the ldap server, should use client certificates generated by the Ldap server RootCA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

http://www.openldap.org/doc/admin24/tls.html

16.1.2. Client Certificates
The DN of a client certificate can be used directly as an authentication DN.
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without the client certificates, dex all requests to ldap server will be rejected.

@liweiv
Copy link
Contributor Author

liweiv commented Sep 7, 2018

Used for LDAP server that used self-signed certificates (RootCA, server cert) and setting TLS verify client. In this case, dex should use the client certificates generated by the Ldap server RootCA.
Without the client certificates, dex all requests to ldap server will be rejected.

http://www.openldap.org/doc/admin24/tls.html

16.1.2. Client Certificates
The DN of a client certificate can be used directly as an authentication DN.
...

srenatus
srenatus reviewed Sep 10, 2018
View reviewed changes
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

One last nitpick -- then I'd be ok with merging this.

I think this would benefit from having a test case added. The existing ldap connector tests start their own slapd instance (that's openldap's server), and test different connection methods. I suppose the one added here (using a client cert) could be a good addition.

However, I don't think this has to block this PR. If you feel like the extra tests are too much, we can create an issue for that, an tackle it later.

connector/ldap/ldap.go Outdated
if c.ClientKey != "" && c.ClientCert != "" {
cert, err := tls.LoadX509KeyPair(c.ClientCert, c.ClientKey)
if err != nil {
return nil, fmt.Errorf("ldap: load self signed certs file failed.error:%s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Can we please make that

fmt.Errorf("ldap: load client cert failed: %v", err)

just to be in line with the updated comment (thanks, btw), and the other error printouts above?

srenatus
srenatus approved these changes Sep 12, 2018
View reviewed changes
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

LGTM. We can expand our tests in a follow-up.

@ericchiang
Copy link
Contributor

FYI there are commits that aren't attributed to the author of this PR. Please squash.

@srenatus
Copy link
Contributor

@veily If you need help squashing commits, let me know! Once squashed, and having only you as author, we'll get this in 🎉

@liweiv
Copy link
Contributor Author

liweiv commented Sep 17, 2018 •
edited
Loading

@srenatus Okay, How squash ? In addition, I used different git username during the completion of this PR, this is my mistake.

@srenatus
Copy link
Contributor

srenatus commented Sep 17, 2018 via email •
edited
Loading

@srenatus
Copy link
Contributor

Update Sorry I was on mobile and hadn't read your reply properly. This is what you'd do in your checkout:

$ git checkout master # just to be sure
$ git rebase -i origin/master # this will open an editor, where you replace the 'pick' with 'squash' in all lines except the first one
$ git commit --amend --no-edit --reset-author # ensure the commit author matches your github settings
$ git push --force YOUR_REMOTE_NAME master

Note that the last line's YOUR_REMOTE_NAME needs to match your remote's name in your local checkout. You can find that in git remote -v, the lines with [email protected]:veily/dex.

@liweiv
support self-signed certificates ldap
317f433 
Format ldap.go

Format ldap.go: with a space for golint

with a space

Rename clientCA is to clientCert

Update ldap.go

modified the ldap client certificate file comments.

modified load ldap client cert error.

modified load ldap client cert error: fmt.Errorf("ldap: load client cert failed: %v", err)
@liweiv
Copy link
Contributor Author

liweiv commented Sep 22, 2018

@srenatus Thanks, I has squashed commits under your guidance.

@srenatus
Copy link
Contributor

Thank you! :)

@srenatus srenatus merged commit ff70c04 into dexidp:master Sep 22, 2018
@srenatus srenatus mentioned this pull request Oct 15, 2018
Closed
mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019
@srenatus
Merge pull request dexidp#1278 from veily/master
5234715 
Support used self-signed certificates LDAP.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srenatus srenatus srenatus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@liweiv @srenatus @ericchiang