-
Notifications
You must be signed in to change notification settings - Fork 256
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
Add support for Wrap Tokens v1 #498
base: master
Are you sure you want to change the base?
Conversation
…e the token; investigating further
…Kafka rejects connection). Investigating
… has correct length byte set
@jcmturner would appreciate if this PR could be reviewed and accepted :) if anything else needs to be done on this one - let me know & I'd try to get it all sorted |
@MikhailMS thanks for this contribution! It looks like the final change set adds wrapTokenV1, but I don't see any additional code upstream that's detecting the format and using the V1 token where needed. Does that still need to be added, or does this just work out of the box through some mechanism I'm not seeing? Thanks again! |
@tzsebe hello there, thank you for the time given to review this PR :) That is correct note - from this library perspective, the change proposed in this PR is all that needed to be done Then, to make use of this new code, upstream application would need to do some work to get the support for V1 token (see this PR I made to Sarama package some time ago) Probably the code change I've proposed in Sarama project could be added to this library instead - I cannot remember why exactly I did it this way, probably to separate concerns (as in actual application needs to know when to use which, not the library to auto-do so for the app) 🤔 |
@MikhailMS thanks for getting back to me! I'm looking at colinmarc's HDFS library, which would need a similar change in a few places. I'm not enough of a SME to know if there's any deterministic way to pick V1 or V2, or if V2 tokens are actually supported by everyone. The Java Kerberos client seems to select between V1 and V2 tokens based on "old" vs "new" encryption schemes:
I don't know if this is a limitation of tokens (where V1 MUST be used for those weak encryption schemes due to format), or some kind of historical backwards-compatibility decision that no longer matters in practice, but if that logic is important, it'd probably be good to incorporate it into GoKRB5 as well. Perhaps it'd be as simple as adding some top level function calls that handle the switching, e.g. We can also be sneaky and compose the v1 into the existing one, delegating to it as needed, which would result in a backwards-compatible change that probably works for every client, but would be kind of a hacky structure. The other option feels cleaner to me. |
This is it I believe; but that would require a bit more code written :) Althought I am not yet seeing a way to generalise this nicely, as some functions are not currently sharing the same signature and there is a diff logic between the versions, of which the caller need to be aware anyway, soo 🤔 (another semi-problem is that I no longer have an access to the setup against which I've tested this; |
Ah, too bad about the setup. Is it hard to recreate? Bringing up a Kerberos-fronted HDFS cluster is easy with dataproc, but I wasn't quite able to configure it to support RC4 in a way that reproduces the issue. The old encryption schemes aren't really supported, unsurprisingly. When I get some time, I'll try to get a deeper understanding of how and why the public interfaces differ between the two. |
Tbh, have no idea; my last attempt at setting up Kerberos + Kafka was back in 2018, and it drove me crazy, so not looking forward trying again :) Functions-wise, the only functions which currently different (as in different signatures) are
we cannot really move the encrypt call out of for And then, while signature is similar, but actual parameters that needs to be passed are different
so, the caller still need to know what Token Version they are working with (albeit we can probably hide this under higher level 🤔 |
Just stumbled across this discussion and maybe what I've discovered can be useful for your further considerations:
So this may give some explanation why "old" ciphers or "new" ciphers are what they are from format perspective. Here you can find some code where WrapToken (with encryption only) and MICToken are implemented for RC4/AES ciphers: https://github.com/oiweiwei/go-msrpc/blob/main/ssp/krb5/crypto/aes_cts_hmac_sha1.go |
This PR:
This was tested against life Cloudera Kafka clusters (multiple versions) and everything is working nicely
I'm not the greatest Go expert, so if there are any issues with the code, let me know & I'd get it fix asap
Really kin to get this one merged, so Sarama can also get patched, which in turn would allow other applications in our stack to receive the fix