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

Fix crash when subject block is empty #7

Merged
merged 3 commits into from
Oct 29, 2017
Merged

Fix crash when subject block is empty #7

merged 3 commits into from
Oct 29, 2017

Conversation

SpencerBrown
Copy link
Contributor

Fixes issue #1 (panic when subject block is empty in tls_cert_request). Return error instead. Same panic occurs in tls_self_signed_cert, fixed that also.

…ead. Same panic occurs in tls_self_signed_cert, fixed that also.
@apparentlymart
Copy link
Contributor

Thanks for fixing this, @SpencerBrown!

The change looks good. I noticed there's some commented-out code in the test. Is that due to the change in certificate? Do you know what it'd take to be able to un-comment that again?

@SpencerBrown
Copy link
Contributor Author

Thanks for reviewing this PR! Yes, this is due to the change in certificate. I used this provider to generate a new hard-coded CA certificate and private key in tls/provider_test.go, because the old one had expired causing the test to fail. This provider is not able to set the AuthorityKeyID field, thus I commented out the test for it.

To enable this test, I would need to either:

  1. Generate the hard-coded test CA cert and private key by other means (e.g. OpenSSL), and include the AuthorityKeyID
  2. Add the ability to set the AuthorityKeyID to this provider, then use it to generate the hard-coded test CA certificate and private key

Option (1) does not seem to progress the cause of this provider. Adding this test back in would not be testing anything that this provider currently handles.

Option (2) seems reasonable to me, but would be better handled by a separate PR implementing the AuthorityKeyID feature, then adding back the test for it. I would be happy to do this as time allows, as it would advance the capabilities of this provider.

What do you think?

@apparentlymart
Copy link
Contributor

Thanks for that additional context, @SpencerBrown!

Given what you explained here, I'm happy to move forward with this code removed altogether and then we can potentially add this back in a later change that also adds support for the feature.

Would you mind updating this to remove that commented-out code (just so it isn't confusing to any future maintainer looking at this code)? I think this is ready to go after that, and we can always find that old code in the version control history if we want to re-introduce it later.

Thanks again for working on this!

@SpencerBrown
Copy link
Contributor Author

Thanks! I removed the commented-out code.

@apparentlymart apparentlymart merged commit acf57bb into hashicorp:master Oct 29, 2017
@apparentlymart
Copy link
Contributor

Thanks @SpencerBrown!

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants