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

tlscommon: update server side verification mode 'full' #242

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

AndersonQ
Copy link
Member

What does this PR do?

Updates server side verification mode 'full'

Why is it important?

To remove unnecessary verification

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

Related issues

  • N/A

@AndersonQ AndersonQ added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Oct 23, 2024
@AndersonQ AndersonQ self-assigned this Oct 23, 2024
@AndersonQ AndersonQ requested a review from a team as a code owner October 23, 2024 08:42
@AndersonQ AndersonQ requested review from belimawr, khushijain21, cmacknz, leehinman and a team and removed request for a team, khushijain21 and belimawr October 23, 2024 08:42
@AndersonQ AndersonQ force-pushed the server-side-verify-full branch from 103772a to 62580ff Compare October 23, 2024 08:46
@pierrehilbert
Copy link


=== RUN   Test_diagError/Server_requires_client_auth
--
  | 2024/10/23 01:55:26 http: TLS handshake error from 127.0.0.1:49263: tls: client didn't provide a certificate
  | diag_test.go:248:
  | Error Trace:	/Users/admin/builds/bk-agent-prod-orka-1729673381016494214/elastic/elastic-agent-libs/transport/httpcommon/diag_test.go:248
  | Error:      	"Get \"https://127.0.0.1:49262\": readLoopPeekFailLocked: remote error: tls: certificate required" does not contain "caused by missing mTLS client cert."
  | Test:       	Test_diagError/Server_requires_client_auth


@AndersonQ AndersonQ force-pushed the server-side-verify-full branch from 62580ff to 2bbc6ab Compare October 24, 2024 12:25
@AndersonQ AndersonQ enabled auto-merge (squash) October 24, 2024 12:26
Type: "CERTIFICATE",
Bytes: cert.Leaf.Raw,
})
require.Errorf(t, err, "failed to encode certificste to PEM")
Copy link
Contributor

Choose a reason for hiding this comment

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

There is nothing to be formatted here:

Suggested change
require.Errorf(t, err, "failed to encode certificste to PEM")
require.Error(t, err, "failed to encode certificste to PEM")

Comment on lines +290 to +295
// VerifyFull would attempt to match 'host' (c.ServerName) that is the host
// the client is trying to connect to with a DNS, IP or the CN from the
// client's certificate. Such validation, besides making no sense on the
// server side also causes errors as the client certificate usually does not
// contain a DNS, IP or CN matching the server's hostname.
case VerifyFull, VerifyCertificate:
Copy link
Contributor

Choose a reason for hiding this comment

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

Conceptually this sounds like an odd default behaviour for me, however I'm not an expert on mTLS.

My main question/concern here is: Is client certificates not containing a hostname a correct behaviour or is it a 'common, but technically wrong practice'?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's the correct behaviour, this check should not be there

@AndersonQ AndersonQ force-pushed the server-side-verify-full branch from 2bbc6ab to 6138278 Compare October 24, 2024 13:11
@AndersonQ AndersonQ merged commit 4babafd into elastic:main Oct 24, 2024
6 checks passed
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @AndersonQ

@AndersonQ AndersonQ deleted the server-side-verify-full branch October 24, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants