clientv3: allow setting JWT directly

[mcrute]

etcd supports using signed JWTs in a verify-only mode where the server has access to only a public key and therefore can not create tokens but can validate them. For this to work a client must not call Authenticate and must instead submit a pre-signed JWT with their request. The server will validate this token, extract the username from it, and may allow the client access.

This change allows setting the JWT directly and not setting a username and password. If a JWT is provided the client will no longer call Authenticate, which would not work anyhow. It also provides a public method UpdateAuthToken to allow a user of the client to update their auth token, for example, if it expires.

In this flow all token lifecycle management is handled outside of the client as a concern of the client user.

[mcrute]

I also added support for passing a token rather than a username and password in etcdctl. I can split this into a separate PR that depends on this one if needed; leaving it here for now though since it's a relatively small change.

[mcrute]

The failing test here appears to be unrelated to this change:

2023-10-22T08:17:25.0015167Z /__w/etcd/etcd/bin/etcd (TestWatchQuorumLastVersion-test-2) (54187): {"level":"error","ts":"2023-10-22T08:17:24.98355Z","caller":"embed/etcd.go:855","msg":"setting up serving from embedded etcd failed.","error":"accept tcp 127.0.0.1:20011: use of closed network connection","stacktrace":"go.etcd.io/etcd/server/v3/embed.(*Etcd).errHandler\n\tgo.etcd.io/etcd/server/v3/embed/etcd.go:855\ngo.etcd.io/etcd/server/v3/embed.(*Etcd).servePeers.func3\n\tgo.etcd.io/etcd/server/v3/embed/etcd.go:607"}

[jmhbnz]

Hey @mcrute - Thanks for raising this idea.

Overall at first glance this seems reasonable. Can you please add a test for this new direct jwt authentication so we can ensure it doesn't break in future? Thanks!

[mcrute]

Hi @jmhbnz, sure thing! I've added some tests for the functionality. Let me know if there's anything else needed to get this merged.

[jmhbnz]

Thanks @mcrute - Quick heads up the etcd team are pretty busy at kubecon this week so there might be a minor delay getting this new feature reviewed. I will take another look when I get some quiet focus time hopefully end of the week 🙏🏻

[mcrute]

Thanks @jmhbnz, I appreciate it. Would really like to get this merged so let me know if there's anything I can do to help move it along. Have a nice KubeCon!

[jmhbnz]

LGTM - Thanks @mcrute.

Would you mind please rebasing this pr onto latest changes in main as I notice make verify is failing on this branch (for an unrelated issue I believe which has since been fixed in main.).

[mcrute]

Thanks for the review, I've rebased main under this so hopefully we'll get a green test run.

[jmhbnz]

/ok-to-test

[jmhbnz]

/test pull-etcd-unit-test

[mcrute]

Looking at the failing test. I don't see how it's immediately related to this change but it's an auth test so I'm suspicious.

[mcrute]

Found the issue. Unconditionally setting authTokenBundle to something non-nil breaks the no-auth tests. Revised to fix this.

[jmhbnz]

Gentle ping @serathius & @ahrtr, I think this one is ready for maintainer review.

@mcrute If we go ahead with this we will probably want to update some documentation on github.com/etcd-io/website , perhaps under:

• https://etcd.io/docs/v3.5/op-guide/authentication/authentication
• https://etcd.io/docs/v3.5/learning/design-auth-v3

[mcrute]

Once this is merged I'll create a PR for the docs. I learned a ton digging through this functionality and definitely would like to make sure others can use it with less effort required.

[ahrtr]

I think we need to add an e2e test as well.

[mcrute]

Have had some time constraints but I'll post an update soon. Adding validation + test case for credential mixes as well as an e2e test validating that standalone token auth works.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

[jmhbnz]

Discussed during sig-etcd triage meeting @mcrute do you have time to rebase this pr and add the e2e test? Would be great to get this merged. Thanks!

[mcrute]

@jmhbnz I'm planning to pick this back up this week, have been distracted by other priorities but would very much like to get this finished and merged.

[mcrute]

Ping @serathius & @ahrtr. Do you have a moment to review (and hopefully merge) this PR? I think that it's complete and incorporates all of the feedback.

[ahrtr]

For this to work a client must not call Authenticate

What will happen if such a client call Authenticate?

[mcrute]

For this to work a client must not call Authenticate

What will happen if such a client call Authenticate?

There will be no change in behavior from the current state of the code. If the server has only a public key it will fail with an error because it will not be able to assign a token to the user. If it has a private key then it will return a new token to the user that they will need to assign to the client themselves.

Although my reading of the code is that this would be a pretty odd case for someone to do this since Authenticate is called automatically in the client getToken method. It is also not called if the client was configured with a Token.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 52.94118% with 8 lines in your changes missing coverage. Please review.

Project coverage is 68.83%. Comparing base (a8b30b1) to head (aa4433d).

❗ Current head aa4433d differs from pull request most recent head 4f46fb4

Please upload reports for the commit 4f46fb4 to get more accurate results.

Files	Patch %	Lines
etcdctl/ctlv3/command/global.go	0.00%	7 Missing ⚠️
client/v3/config.go	50.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files	Coverage Δ
client/v3/client.go	85.25% <100.00%> (+0.35%)	⬆️
etcdctl/ctlv3/ctl.go	0.00% <ø> (ø)
client/v3/config.go	85.00% <50.00%> (+0.38%)	⬆️
etcdctl/ctlv3/command/global.go	0.00% <0.00%> (ø)

... and 23 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #16803      +/-   ##
==========================================
+ Coverage   68.81%   68.83%   +0.01%     
==========================================
  Files         420      420              
  Lines       35490    35504      +14     
==========================================
+ Hits        24422    24438      +16     
+ Misses       9637     9634       -3     
- Partials     1431     1432       +1     

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8b30b1...4f46fb4. Read the comment docs.

[mcrute]

Rebasing this was a terrible idea 😭. Undoing that bad decision and hopefully these new, unrelated, tests failures disappear.

[ahrtr]

Rebasing this was a terrible idea

Please rebase this PR, because it is based on a commit of 3 weeks ago. We ran into conflict which wasn't detected by github.

Two generic comments/questions,

• Do you have a real or strong production use case for this change?
• Users need to manage the token themselves. In that case, etcd (client sdk) should never automatically retry when it gets an invalid auth failure response. But unfortunately we don't support disabling the retry for now. Refer to the "Proposal" section in etcd Go & Java client SDK's retry mechanism may break Serializable #18424.

[mcrute]

Rebasing this was a terrible idea

Please rebase this PR, because it is based on a commit of 3 weeks ago. We ran into conflict which wasn't detected by github.

Once I have a clean run of all the tests I'll rebase. The first rebase introduced a lot of seemingly spurious failures and I would like to make sure I've got completely working and tested code before digging into those.

Two generic comments/questions,

• Do you have a real or strong production use case for this change?

Yes, we use some code similar to this for application configuration and secret bootstrapping. The application runner injects a pre-signed etcd token into the application environment when it starts then a library within the application discovers etcd (using DNS SRV records), connects with the injected token, and reads its configuration and secrets from etcd. There is a separate controller that handles token lifetimes by creating new tokens and passing them to applications over a pre-defined key in etcd so the application can update the token for their etcd connection before expiration (it's a little more complicated than that but that's the general workflow).

• Users need to manage the token themselves. In that case, etcd (client sdk) should never automatically retry when it gets an invalid auth failure response. But unfortunately we don't support disabling the retry for now. Refer to the "Proposal" section in etcd Go & Java client SDK's retry mechanism may break Serializable #18424.

At least for our use-case that should be okay. We swap tokens well in advance of their expiration. Although having this functionality would be a good safety measure.

[mcrute]

Looks like the build was clean. Going to push the rebase now for testing.

[mcrute]

@ahrtr all right, I've got all the requested changes incorporated and we've got a clean build with the rebase! Let me know if there's anything else you'd like to see or if we're good to merge this. Thanks.

[ahrtr]

There is a separate controller that handles token lifetimes by creating new tokens and passing them to applications over a pre-defined key in etcd so the application can update the token for their etcd connection before expiration

If the application doesn't update the token before expiration in time for whatever reason, then you won't have any chance to know the authRevision anymore (of course you can get it by hack way) because you don't have a valid token; but you won't be able to generate a new JWT token because you don't know the latest authRevision. You will run into a deadlock.

One possible solution is to loosen the access control on AuthStatus so that you can always get the latest authRevision no matter whether or not you have a valid token,

etcd/client/v3/auth.go

Line 70 in 3aa56a7

AuthStatus(ctx context.Context) (*AuthStatusResponse, error)

Refer to,

etcd/server/etcdserver/apply/apply_auth.go

Lines 175 to 176 in 3aa56a7

	case r.AuthStatus != nil:
	return true

Also we need to update the doc to clarify the use case/workflow for this change.

[mcrute]

Just checking back in on this. I'm open to removing loosening the access control on AuthStatus if you think that's a valid approach. I don't think it would reveal anything security sensitive other than possibly the timing of auth store changes. If you're +1 on this idea then I'll implement it as another commit on this PR.

I will also update the docs but since those are in a different repo I was planning to do that after this was merged. Do you want me to open a PR against the docs as well and cross-link them?

I'd really like to get this shipped in September, if possible, so we can stop vendoring a fork of this library.

[ahrtr]

Overall this is a valid & minor change (only client side change) to me. Followups,

• Loosen the access control on AuthStatus as mentioned in clientv3: allow setting JWT directly #16803 (comment). Personally I think that we should loosen it even without this PR. The purpose of AuthStatus is to check whether auth is enabled or not, it shouldn't require a valid token to access it, otherwise it's a chicken-egg issue.
• Once the JWT token is managed by users themselves, then it's out of etcd's control, so the client (SDK) should never automatically retry when it gets an invalid auth failure response as mentioned in clientv3: allow setting JWT directly #16803 (comment). This can be resolved separately. We just need to expose an API to let users to disable the retry mechanism.
• Update doc (yes, please raise a separate ticket once we have a consensus)

It's hard to track all these tasks across PRs. Could you raise a ticket to track them?

cc @serathius @jmhbnz @ivanvc

[ivanvc]

Minor observation/question: thinking about the work we have been doing with errors, does setting this error as an exported variable make sense? Would it be valuable? WDYT @ahrtr?

[ahrtr]

Sounds like a good suggestion.

[ivanvc]

Given the number of follow-ups we have for this pull request, and the time it has been open, I think it's okay to do this in a follow-up PR.

[ivanvc]

Given that this will be exported and it may be difficult to change the name later on, I feel like "Token" may be too generic. Would "JWT" be a better name (more implementation specific)?

[ahrtr]

Theoretically, it can be any token types that are supported by etcd. The etcdserver just delegates to the related token provider to parse the token based on the token type configured in --auth-token.

But in practice, it's JWT for now. So suggest to keep using Token

[ivanvc]

LGTM. Thanks, Mike.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivanvc, jmhbnz, mcrute

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jmhbnz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.