Skip to content
This repository has been archived by the owner on Nov 24, 2021. It is now read-only.

Support service principal with slash #2

Closed
wants to merge 1 commit into from
Closed

Support service principal with slash #2

wants to merge 1 commit into from

Conversation

kristian-lesko
Copy link
Contributor

Service principals containing a slash (/) prompt an error
when looking for the key in the Keytab.DecryptEncPart method
because the principal is not divided into components.

Closes #1.

Service principals containing a slash (/) prompt an error
when looking for the key in the Keytab.DecryptEncPart method
because the principal is not divided into components.
@CLAassistant
Copy link

CLAassistant commented Mar 23, 2018

CLA assistant check
All committers have signed the CLA.

@ah-
Copy link
Contributor

ah- commented Mar 28, 2018

Thanks for this!

I'll have a look in a bit, I think the fix is correct but need to find where to best do the splitting. If the fix ends up being in gokrb we should fix it upstream rather than in our vendored copy.

@kristian-lesko
Copy link
Contributor Author

@ah- thank you; I wasn't sure if this applies to gokrb in general, or just this plugin. If upstream happens to be the best place to fix, feel free to forward the patch there.

@ah-
Copy link
Contributor

ah- commented Apr 20, 2018

Just had a closer look and your solution seems exactly right! I've opened a PR with this change upsteam: jcmturner/gokrb5#105

Once this is merged we can update our gokrb5 dependency and have this fixed.

@kristian-lesko
Copy link
Contributor Author

Thanks a lot for helping this patch to propagate upstream, it's great to see that it's already merged there.

I'm looking forward for when the gokrb5 dependency is updated here; this plugin is tremendously useful, and not having to rebuild it with this patch anymore will be much more practical. Thank you! :-)

@ah-
Copy link
Contributor

ah- commented Apr 24, 2018

Everything got merged now and the new 1.1.0 release should properly support this now thanks to your patch! We've also updated our vault dependency to 0.10, so you might have to update yours as well if you want to switch to it, sadly it doesn't seem easy to support both versions at the same time.

@ah- ah- closed this Apr 24, 2018
@kristian-lesko
Copy link
Contributor Author

@ah- Thanks a lot!

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

Successfully merging this pull request may close these issues.

3 participants