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.

Sign up for GitHub

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

Jump to bottom

CA trusted fingerprint tests #29347

Merged
merged 3 commits into from
Dec 15, 2021
Merged

CA trusted fingerprint tests #29347

merged 3 commits into from
Dec 15, 2021

Conversation

belimawr
Copy link
Contributor

@belimawr belimawr commented Dec 8, 2021 •
edited
Loading

What does this PR do?

This PR adds tests for when ssl.ca_trusted_fingerprint is set

Why is it important?

It increases our test coverage and ensures the implementation is correct.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
    - [ ] I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
    - [ ] I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

How to test this PR locally

  1. go to libbeat/common/transport/tlscommon
  2. run the tests: go test ./...

Related issues

Use cases

Screenshots

Logs

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Dec 8, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 8, 2021

This pull request does not have a backport label. Could you fix it @belimawr? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Dec 8, 2021
@belimawr belimawr marked this pull request as ready for review December 8, 2021 14:48
@belimawr belimawr requested a review from kvch December 8, 2021 14:48
@belimawr belimawr added libbeat review Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Dec 8, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Dec 8, 2021
@belimawr belimawr added the backport-v8.0.0 Automated backport with mergify label Dec 8, 2021
@mergify mergify bot removed the backport-skip Skip notification from the automated backport with mergify label Dec 8, 2021
kvch
kvch reviewed Dec 8, 2021
View reviewed changes
libbeat/common/transport/tlscommon/tls_config_test.go Outdated
@@ -49,15 +47,15 @@ func TestMakeVerifyServerConnection(t *testing.T) {
peerCerts []*x509.Certificate
serverName string
expectedCallback bool
expectedError error
expectedError bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The validation using this field as an error wasn't quite correct, it used be like this:

assert.Error(t, test.expectedError, err)

However assert.Error's signature is: (t TestingT, err error, msgAndArgs ...interface{}) bool, hence the test was validating that the test case had an error set and using the returned error as a message.

I did try for a wee bit to get one of the other error functions provided by testify to assert the error type, but they didn't quite work, nor I invested much time on it. In the end it was simpler and quicker to just test for error/no error.

Does it make sense? Do we need to be more specific while checking those errors?

Testify can be quite hard to use...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've improved the tests to also validate the error type and message on bc05440788.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Dec 8, 2021 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-12-15T11:59:49.109+0000

  • Duration: 26 min 43 sec

  • Commit: 5c7ebfb

Test stats 🧪

Test Results
Failed 0
Passed 980
Skipped 190
Total 1170

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@belimawr belimawr requested a review from kvch December 9, 2021 15:07
@belimawr
tlscommon: add tests for ssl.ca_tursted_fingerprint
8088558 
This commit adds tests to `trustRootCA` and `makeVerifyConnection`,
the tests aim to cover the usage of `ssl.ca_tursted_fingerprint`.
@belimawr
tlscommon: refactor tests
4e34351 
* `openTestCerts` receives a `testing.TB` and fail tests in case of an
error
* fixing testify usage
@belimawr belimawr force-pushed the ca_trusted_fingerprint_tests branch from 42c1a0f to bc05440 Compare December 13, 2021 15:12
@belimawr
Copy link
Contributor Author

rebase onto master, force push

@belimawr belimawr force-pushed the ca_trusted_fingerprint_tests branch from bc05440 to 5c7ebfb Compare December 13, 2021 15:18
@belimawr
Copy link
Contributor Author

/test

2 similar comments
@belimawr
Copy link
Contributor Author

/test

@belimawr
Copy link
Contributor Author

/test

@belimawr
Copy link
Contributor Author

/test

@belimawr belimawr merged commit 6c268f8 into elastic:master Dec 15, 2021
@belimawr belimawr deleted the ca_trusted_fingerprint_tests branch December 15, 2021 14:22
mergify bot pushed a commit that referenced this pull request Dec 15, 2021
@belimawr
CA trusted fingerprint tests (#29347)
a10272e 
This commit adds tests to `trustRootCA` and `makeVerifyConnection`,
the tests aim to cover the usage of `ssl.ca_tursted_fingerprint`.

Some existing tests were refactored, `openTestCerts` receives a `testing.TB`
and fail tests in case of an error

(cherry picked from commit 6c268f8)
@mergify mergify bot mentioned this pull request Dec 15, 2021
Merged
belimawr added a commit that referenced this pull request Dec 16, 2021
@mergify @belimawr
CA trusted fingerprint tests (#29347) (#29451)
3e564f6 
This commit adds tests to `trustRootCA` and `makeVerifyConnection`,
the tests aim to cover the usage of `ssl.ca_tursted_fingerprint`.

Some existing tests were refactored, `openTestCerts` receives a `testing.TB`
and fail tests in case of an error

(cherry picked from commit 6c268f8)

Co-authored-by: Tiago Queiroz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvch kvch kvch approved these changes

Assignees
No one assigned
Labels
backport-v8.0.0 Automated backport with mergify libbeat review Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@belimawr @elasticmachine @kvch