-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support using TLS cert subject to auth user #896
Conversation
@@ -144,6 +144,12 @@ func (s *Server) checkAuthforWarnings() { | |||
warn = true | |||
} | |||
for _, u := range s.users { | |||
// Skip warn if using TLS certs based auth | |||
// unless a password has been left in the config. | |||
if u.Password == "" && s.opts.TLSMap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This removes the following warning for when TLS map feature is used for auth:
[3331] 2019/02/04 11:37:04.713687 [WRN] Plaintext passwords detected, use nkeys or bcrypt.
if c.opts.Username != "" { | ||
s.Warnf("User found in connect proto, but user required from cert - %v", c) | ||
} | ||
// Already checked that the client didn't send a user in connect | ||
// but we set it here to be able to identify it in the logs. | ||
c.opts.Username = euser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added reusing the username field here to be able to log on auth violation, otherwise would become User "N/A"
[1351] 2019/02/04 08:56:10.697602 [ERR] 127.0.0.1:49558 - cid:1 - Publish Violation - User "N/A", Subject "discovery.pRvIVQCPUPtGP34OTGiyBf.status"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - We should check if this works for MC. Also we need to document how to config the users in the config file, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
if len(cert.EmailAddresses) == 0 { | ||
|
||
hasEmailAddresses := len(cert.EmailAddresses) > 0 | ||
hasSubject := len(cert.Subject.String()) > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should document how this forces a specific format for the config files.
FYI - I'll share this PR with the interested parties. |
Can you resolve the conflicts and then merge? Or do I need to merge? |
Signed-off-by: Waldemar Quevedo <wally@synadia.com>
I'll rebase then merge, and will add docs for using this with |
46d2962
to
7645d95
Compare
Thanks.. |
Adds support for checking a TLS cert subject to be used for auth besides email address when
verify_and_map
is enabled.git pull --rebase origin master
)Signed-off-by: Waldemar Quevedo wally@synadia.com
/cc @nats-io/core