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

Conditionally overwrite TLS parameters for MySQL secrets engine #9729

Merged
merged 3 commits into from Aug 17, 2020
Merged

Conditionally overwrite TLS parameters for MySQL secrets engine #9729

merged 3 commits into from Aug 17, 2020

Conversation

0x63lv
Copy link
Contributor

@0x63lv 0x63lv commented Aug 13, 2020

Changes to how connections to MySQL secrets engine are set up, which were introduced with #9181 and released with Vault 1.5.0, broke a working setup in Vault versions prior to 1.5.0, where a valid TLS parameter was set in MySQL DSN (e.g. ...?tls=true).

If none of the new parameters introduced with 1.5.0 (tls_ca or tls_certificate_key) would be set, the TLS configuration would be empty, and it would be written over the existing TLS parameters in the DSN (e.g. tls=true), resulting in a non-TLS connection attempt to MySQL.

This PR would change that behaviour, and only overwrite the TLS parameters in the DSN, if at least one of the tls_ca or tls_certificate_key parameters are set. Otherwise it would leave the TLS configuration set in DSN as-is.

Tests also updated with this case.

Overwrite MySQL TLS configuration in MySQL DSN only if have `tls_ca` or `tls_certificate_key` set
Current logic always overwrites it
Copy link
Contributor

@Valarissa Valarissa left a comment

Choose a reason for hiding this comment

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

LGTM

@kalafut kalafut added this to the 1.5.1 milestone Aug 17, 2020
@Valarissa Valarissa merged commit 48db6d3 into hashicorp:master Aug 17, 2020
catsby added a commit that referenced this pull request Aug 18, 2020
* master:
  Add a section to the MySQL secrets plugin docs about x509 (#9757)
  Update documentation for MySQL Secrets Engine (#9671)
  Conditionally overwrite TLS parameters for MySQL secrets engine (#9729)
  Correctly mark Cassandra as not supporting static roles (#9750)
  changelog++
  pki: Allow to use not only one variable during templating in allowed_domains #8509 (#9498)
  agent/templates: update consul-template to v0.25.1 (#9626)
  Restoring the example policies for blocking sha1 (#9677)
  changelog++
  changelog++
  Document the new SSH signing algorithm option. (#9197)
  CHANGELOG-+
  CHANGELOG++
  Trail of bits 018 (#9674)
@0x63lv
Copy link
Contributor Author

0x63lv commented Sep 30, 2020

Would it be possible to know, when this change will be in a public release?

@Valarissa
Copy link
Contributor

Valarissa commented Oct 1, 2020

My sincerest apologies. It looks like this was slated for an earlier release but the backport got bungled and didn't make it into the release.

@Valarissa Valarissa modified the milestones: 1.5.1, 1.5.4, 1.5.5 Oct 1, 2020
@Valarissa
Copy link
Contributor

We'll be releasing it as part of the 1.5.5 release which should be in the next couple weeks. Once again, apologies that this wasn't released sooner.

Valarissa pushed a commit that referenced this pull request Oct 1, 2020
* Conditionally overwrite TLS parameters in MySQL DSN

Overwrite MySQL TLS configuration in MySQL DSN only if have `tls_ca` or `tls_certificate_key` set
Current logic always overwrites it

* Add test for MySQL DSN with a valid TLS parameter in query string
Valarissa pushed a commit that referenced this pull request Oct 1, 2020
… (#10073)

* Conditionally overwrite TLS parameters in MySQL DSN

Overwrite MySQL TLS configuration in MySQL DSN only if have `tls_ca` or `tls_certificate_key` set
Current logic always overwrites it

* Add test for MySQL DSN with a valid TLS parameter in query string

Co-authored-by: arnis <8789226+0x63lv@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants