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

Update TLS certificates in tests #169

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Conversation

belimawr
Copy link
Contributor

What does this PR do?

Some TLS certificates used in tests expired, this commit fixes it by generating the certificates and, if needed, calculating the fingerprint on each test.

This will prevent future CI failures and reduce the maintenance burden.

Why is it important?

It fixes the broken tests

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
    - [ ] I have added tests that prove my fix is effective or that my feature works
    - [ ] I have added an entry in CHANGELOG.md

## Author's Checklist
## Related issues

@belimawr belimawr added bug Something isn't working Team:Elastic-Agent Label for the Agent team labels Dec 19, 2023
@belimawr belimawr self-assigned this Dec 19, 2023
@belimawr belimawr requested a review from a team as a code owner December 19, 2023 18:31
@belimawr belimawr requested review from rdner and fearful-symmetry and removed request for a team December 19, 2023 18:31
Some TLS certificates used in tests expired, this commit fixes it by
generating the certificates and, if needed, calculating the
fingerprint on each test.

This will prevent future CI failures and reduce the maintenance
burden.
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for fixing it where it won't need to be updated again.

transport/tlscommon/tls_config_test.go Outdated Show resolved Hide resolved
}

// If for any reason there is a need to debug
Copy link
Contributor

Choose a reason for hiding this comment

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

If for some reason this fails in CI, we are going to want the cert/key pair to see what happened. How about always dumping out the keys & certs, then deleting the dir if the test succeeds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I first thought about it, but while writing this PR what helped me was to have a fixed set of certs that I could use over multiple test runs. So I ended up generating them once and re-using until I found out where the mismatch between cert, CA and fingerprint were.

Anyway, I see no issue in persisting them on failure.

transport/tlscommon/ca_pinning_test.go Outdated Show resolved Hide resolved
@belimawr
Copy link
Contributor Author

All suggestions implemented on 9d031ae

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @belimawr

@belimawr belimawr merged commit b200fb1 into elastic:main Dec 21, 2023
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants