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

if auth is enabled, what takes precedence user/pass or CN from client-cert #8584

Closed
raoofm opened this issue Sep 20, 2017 · 13 comments
Closed
Assignees

Comments

@raoofm
Copy link
Contributor

raoofm commented Sep 20, 2017

Just wanted to check if v3 auth is enabled and client-cert-auth is enabled

As mentioned here

If an etcd server is launched with the option --client-cert-auth=true, the field of Common Name (CN) in the client's TLS cert will be used as an etcd user. In this case, the common name authenticates the user and the client does not need a password.

What is the expected behavior if client cert and user/pass both are presented? Which one of these two take priority?

@gyuho
Copy link
Contributor

gyuho commented Sep 20, 2017

The CN will take priority. And user/password will be ignored.

@gyuho gyuho closed this as completed Sep 20, 2017
@raoofm
Copy link
Contributor Author

raoofm commented Sep 21, 2017

@gyuho

https://github.com/coreos/etcd/blob/master/etcdserver/api/v2http/client_auth.go#L88

package name shows v2http, i'm interested in v3 auth behavior.

@raoofm
Copy link
Contributor Author

raoofm commented Sep 21, 2017

@gyuho @xiang90 @heyitsanthony
Isn't this a very odd behavior and shouldn't this be changed.
I agree that clientCertAuth should be there but not necessarily that should define the behavior of auth when accessing keys.

For auth v3 the flow can be:

  • Check clientCertAuth is met
  • Check if Authorization token is provided
    • if yes then get AuthInfoFromToken
  • If not get AuthInfoFromTLS
  • else fail

If an auth token is passed then the token should take priority not cert. This simplifies the management of client user/roles wrather than client cert CN.

A common client cert be shared per team with each app having its own basic auth.

Usecase:
Probably I can set up a proxy and have a client cert for it to talk to the cluster and all clients can reach the cluster via proxy with their auth credentials.

@gyuho
Copy link
Contributor

gyuho commented Sep 21, 2017

I think the suggested workflow would break backward compatibility.
/cc @mitake (original author of the patch #6881).

@raoofm
Copy link
Contributor Author

raoofm commented Sep 21, 2017

@gyuho @hongchaodeng @xiang90 @heyitsanthony but this means, users who enabled v3 auth with basicAuth can never enable client-cert-auth even if they want to as they cannot migrate all the clients at the same time.

Also users who were managing their etcd clusters now want to migrate to etcd-operator but etcd-operator enables client-cert-auth by default on tls and the existing clients would break as basicAuth will be ignored.

I think the suggested workflow would break backward compatibility.

why do you think it breaks backward compatibility? I don't see anywhere in documentation it is mentioned that if client-cert-auth is enabled then passed --user is ignored. Basic auth and client cert are never mentioned together.

@raoofm
Copy link
Contributor Author

raoofm commented Sep 21, 2017

@gyuho do you think it is worth to keep this issue open?

@gyuho gyuho reopened this Sep 21, 2017
@xiang90
Copy link
Contributor

xiang90 commented Sep 21, 2017

I believe username and password specified in the request should take priority over CN in the cert. If it is not the case, we need to make it happen somehow without breaking compatibility.

@xiang90
Copy link
Contributor

xiang90 commented Sep 21, 2017

/cc @mitake

@raoofm
Copy link
Contributor Author

raoofm commented Sep 22, 2017

@xiang90 thank you

@raoofm
Copy link
Contributor Author

raoofm commented Sep 22, 2017

@xiang90

I believe username and password specified in the request should take priority over CN in the cert. If it is not the case, we need to make it happen somehow without breaking compatibility.

@gyuho

I think the suggested workflow would break backward compatibility.

I still don't understand the breaking of the compatibility part.

Let me pen it down and attempt to make things more clear. ☕

Current Behavior

As per docs

If an etcd server is launched with the option --client-cert-auth=true, the field of Common Name (CN) in the client's TLS cert will be used as an etcd user. In this case, the common name authenticates the user and the client does not need a password.

As per code
etcdserver/v3_server.go

func (s *EtcdServer) AuthInfoFromCtx(ctx context.Context) (*auth.AuthInfo, error) {
	if s.Cfg.ClientCertAuthEnabled {
		authInfo := s.AuthStore().AuthInfoFromTLS(ctx)
		if authInfo != nil {
			return authInfo, nil
		}
	}
	return s.AuthStore().AuthInfoFromCtx(ctx)
}

Implies:

  1. With the current doc and code, if anyone ever enabled client-cert-auth then eventually it left them with unusable basic auth.
  2. So today there might be a very small set (or 0) users who are sending basic auth along with client-cert-auth (simply because it doesn't make sense nor will it work).
  3. It is safe to enable a feature which says if token comes in Authorization header then CN is ignored.

We can argue that point number 2 is an assumption and we should not break even a single user who is making a mistake of sending Authorization header even though it is never used/worked.

So if we really want to cover up others mistakes then we can add a new flag something like --prioritize-token-over-cn(ETCD_PRIORITIZE_TOKEN_OVER_CN) which should be false by default and upon enabling the order first checks token and if not found gets user from CN

IMHO a flag is unnecessary here. We can just document it as a warning in the release notes. Feel free to disagree.

@mitake
Copy link
Contributor

mitake commented Sep 22, 2017

@raoofm thanks for pointing out. I agree with changing the priority. I also think the new flag for configuring priority is overkill because prioritizing password based auth is natural (although I'm not fully sure about this point).

@mitake mitake self-assigned this Sep 22, 2017
@raoofm
Copy link
Contributor Author

raoofm commented Sep 22, 2017

@mitake thanks

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

No branches or pull requests

4 participants