-
Notifications
You must be signed in to change notification settings - Fork 109
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
Format key in the same way as certificate #175
base: main
Are you sure you want to change the base?
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
Hi @samwmarsh. To help provide context to these changes, would you mind please editing the PR to describe the changes made and include links to an issue if it is related? Thank you for the contribution! |
@codydbentley updated! Let me know if you need anything else |
@codydbentley we've done some testing this morning and seen the following (which makes me sad and this PR invalid, but also raises a bug) Test Case 1 Test Case 2 Test Case 3 Test cases 2 and 3 were where we were before raising this PR, with the idea that the \n character in the BEGIN PRIVATE KEY and END PRIVATE KEY lines were causing this error, however as proven with test case 1, this is not the case. In fact, the kernel panic happens in both cases 1 and 3. As a note, this [panic recovery] references BloodHound/packages/go/crypto/tls.go Line 40 in 00668cc
To generate key/certificate we're just simply doing
And using cert.crt as the certificate, key.pem as the key. This is confirmed working as described in Test Case 2. |
@samwmarsh - we have some work upcoming in this area that will allow us to review and get this across the line properly. Our apologies for the delay in merging! |
@samwmarsh Apologies for the slow review and thank you for your patience. I noticed there are new suggestions mentioned in #83, have you tried them out by chance? Specifically, this comment: #83 (comment). The code changes in this pull request are well contained and clear though it looks like the newly added Let us know if you try the suggestions mentioned here #83 (comment) and if it works or not for you. Thank you! |
Hi @urangel, Thanks, |
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.
Just want to see the runway -->
Description
Currently only the certificate has a prefix and suffix added if it doesn’t exist. This is to bring the SAML key inline.
Motivation and Context
In #83 we’ve been having issues with SAML working when not using AAD as the identity provider. We’ve been able to get this working with values within the config.json, however we’re currently getting issues when passing in environment variables. We’re concerned that the \n in the key is causing it to be interpreted incorrectly when being passed as an env var in Helm/K8s so we’d like to add this to attempt to resolve this issue.
How Has This Been Tested?
Currently tested same logic already works for certificate. I’m unaware of the impacts, I’m new to golang and it’s quite confusing to attempt to follow so please test on my behalf!
Screenshots (if appropriate):
Types of changes
Checklist: