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

tls: Add descriptions for x509v3 alt name verification (tls.verify_hostname) #1393

Merged
merged 3 commits into from
Jun 27, 2024

Conversation

cosmo0920
Copy link
Contributor

Corresponding to fluent/fluent-bit#8966.

Copy link
Contributor

@patrick-stephens patrick-stephens left a comment

Choose a reason for hiding this comment

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

Is there a reason we default to not verifying the alternative name? This seems counter to an expectation of secure by default.

@@ -9,6 +9,7 @@ Both input and output plugins that perform Network I/O can optionally enable TLS
| :--- | :--- | :--- |
| tls | enable or disable TLS support | Off |
| tls.verify | force certificate validation | On |
| tls.verify\_hostname | force hostname validation for certificates | Off |
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly different wording to below.

administration/transport-security.md Outdated Show resolved Hide resolved
administration/transport-security.md Outdated Show resolved Hide resolved
administration/transport-security.md Outdated Show resolved Hide resolved
Co-authored-by: Pat <[email protected]>
Signed-off-by: Hiroshi Hatake <[email protected]>
@cosmo0920
Copy link
Contributor Author

cosmo0920 commented Jun 21, 2024

Is there a reason we default to not verifying the alternative name? This seems counter to an expectation of secure by default.

Once, I implemented this feature as a forcibly style, I just though such way. Because hostname verification is mostly required for TLS connections to prevent malformed hostname attached instances. However, we got realized that there is many issues in certificates for kubelet. So, preserving the backward compatibilities, we need to disable by default.

@patrick-stephens
Copy link
Contributor

Is there a reason we default to not verifying the alternative name? This seems counter to an expectation of secure by default.

Once, I implemented this feature as a forcibly style, I just though such way. Because hostname verification is mostly required for TLS connections to prevent malformed hostname attached instances. However, we got realized that there is many issues in certificates for kubelet. So, preserving the backward compatibilities, we need to disable by default.

Is this just to handle kubelet specific things then? I'd expect tls verify for everything else, e.g. outputs, etc., so can we just add a special config for kubelet usage to disable it?

@cosmo0920
Copy link
Contributor Author

Is there a reason we default to not verifying the alternative name? This seems counter to an expectation of secure by default.

Once, I implemented this feature as a forcibly style, I just though such way. Because hostname verification is mostly required for TLS connections to prevent malformed hostname attached instances. However, we got realized that there is many issues in certificates for kubelet. So, preserving the backward compatibilities, we need to disable by default.

Is this just to handle kubelet specific things then? I'd expect tls verify for everything else, e.g. outputs, etc., so can we just add a special config for kubelet usage to disable it?

Hmm.., I see. For 3.x line, we need to restore the behavior. But it's reasonable to verify certificates as much as possible in 3.1.
@edsiper Can we break the behavior around certificates in Fluent Bit 3.1? It's reasonable to verify certificates except for filter_kubernetes.

@cosmo0920
Copy link
Contributor Author

Oh, merged my PRs as-is. Also, users complains in in_forward and filter_kubernetes. This is because our network stack is not dumped for its error status with human readable errors. So, we need to restore the behavior. For TLS client, we can print the error message from OpenSSL's certificate verification. But, for server, we didn't get how to print an error with human readable modifications.

@lecaros lecaros merged commit 397a33a into master Jun 27, 2024
4 checks passed
@lecaros lecaros deleted the cosmo0920-x509v3-alt-name-verification branch June 27, 2024 22:24
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.

3 participants