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

[configtls] Validate MinVersion and MaxVersion #9475

Closed
mx-psi opened this issue Feb 6, 2024 · 6 comments
Closed

[configtls] Validate MinVersion and MaxVersion #9475

mx-psi opened this issue Feb 6, 2024 · 6 comments

Comments

@mx-psi
Copy link
Member

mx-psi commented Feb 6, 2024

We effectively restrict the allowed versions to the set defined here:

var tlsVersions = map[string]uint16{

We should validate for it! This can be done after 1.0 per the rules we have here

* **Making validation rules more strict**. A valid configuration struct as defined by its `Validate` method return value
must continue to be valid after a change to the validation rules, except when the configuration struct would cause an error
on its intended usage (e.g. when calling a method or when passed to any method or function in any module under opentelemetry-collector).

Still, it would be nice to have before 1.0 IMO

@molejnik88
Copy link
Contributor

molejnik88 commented Feb 25, 2024

Hi,
Isn't MinVersion validated here: configtls.go#L176 and MaxVersion validated here: configtls.go#L180?

Do we want to call LoadTLSConfig in respective components' config.Validate() methods?

@TylerHelmuth
Copy link
Member

@molejnik88 we should add a Validate function to TLSSetting. When a component's config uses TLSSetting and Validate is called for the component's config, it will also call TLSSetting's Validate.

@yurishkuro
Copy link
Member

When a component's config uses TLSSetting and Validate is called for the component's config, it will also call TLSSetting's Validate.

@TylerHelmuth are you saying this would be automatic, or does each config's Validate need to call TLSSettings.Validate() explicitly?

@TylerHelmuth
Copy link
Member

As long as I am reading this correctly, it is automatic. The component framework will call validate on the entire config, which ends up making Validate calls on anything it can within the config struct.

@molejnik88
Copy link
Contributor

I'd like to work on this issue. Can you please assign it to me?

@atoulme
Copy link
Contributor

atoulme commented Feb 26, 2024

Done, go for it

dmitryax pushed a commit that referenced this issue Mar 7, 2024
**Description:**
Add `Validate()` method to `TLSSetting` and validate tls `min_version`
and `max_version`.

**Link to tracking Issue:** 
#9475
@TylerHelmuth TylerHelmuth moved this to Done in Collector: v1 Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

6 participants