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

allow to skip TLS check in acme http-01 challenge #22521

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

rubroboletus
Copy link
Contributor

solves #22074 allows to set "tls_skip" on pki acme, which will skip TLS check on acme http-01 challenges.

@rubroboletus rubroboletus requested a review from a team as a code owner August 23, 2023 12:13
@hashicorp-cla
Copy link

hashicorp-cla commented Aug 23, 2023

CLA assistant check
All committers have signed the CLA.

@schavis schavis added the needs-eng-review Community PR waiting for ENG review label Aug 31, 2023
@kitography
Copy link
Contributor

Hi @rubroboletus - thanks for the PR! Sorry for the delay here.

We've taken a look at this internally:

  • Currently this pull request produces different behaviour for PKI secrets engines that are created under this code compared to those that are migrated to the version using this code. In particular, the default behaviour is "true", but go-booleans default to false. We want to avoid behaviour where a mounts default values differ depending on which version of vault a mount was created - rather than which version they are currently running. This sort of inconsistency is known as particularly hard for users and for our support staff to understand and debug.
  • Having checked with internal stakeholders, we don't think it makes sense to require TLS verification when that is from a redirect from http. In particular, it's no less secure than http. That reduces the amount of configuration necessary which would be really nice.

If you want to take a look at simply skipping TLS verify in the redirect case (removing configuration), we'll accept that PR. If
you don't get to this before Friday November 10th, I'll take a stab at it.

Thanks!

@kitography kitography self-assigned this Oct 30, 2023
@rubroboletus
Copy link
Contributor Author

Hi @kitography,

ok, I have removed all the configurable logic and just simply ignore TLS.

Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rubroboletus Thanks for the PR, much appreciated and sorry about the delay getting this merged.

I've added a test case so we don't accidentally break this in the future.

@stevendpclark stevendpclark merged commit 28e3507 into hashicorp:main Nov 15, 2023
101 of 102 checks passed
@stevendpclark stevendpclark added this to the 1.14.7 milestone Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cryptosec needs-eng-review Community PR waiting for ENG review secret/pki
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants