Skip to content

Validate TLS config: return error if cert or key is missing #13134

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .chloggen/fix-tls-validation.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
change_type: changed
component: configtls
note: Validate TLS config earlier: return error if certificate or key is missing.
issues: [13134]
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,12 @@ This release includes 2 very important breaking changes.

- `configrpc`: Use own compressors for zstd. Before this change, the zstd compressor we used didn't respect the max message size. This addresses [GHSA-c74f-6mfw-mm4v](https://github.com/open-telemetry/opentelemetry-collector/security/advisories/GHSA-c74f-6mfw-mm4v) for `configgrpc` (#10323)

# Unreleased

### Changed
- Validate TLS config earlier: return error if certificate or key is missing. ([#13134](https://github.com/open-telemetry/opentelemetry-collector/pull/13134))
g

## v1.9.0/v0.102.0

**This release addresses [GHSA-c74f-6mfw-mm4v](https://github.com/open-telemetry/opentelemetry-collector/security/advisories/GHSA-c74f-6mfw-mm4v) for `confighttp`.**
Expand Down
70 changes: 70 additions & 0 deletions config/configtls/config_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

package configtls

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestConfig_Validate_CertKeyPresence(t *testing.T) {
tests := []struct {
name string
config Config
expectErr bool
}{
{
name: "no cert or key",
config: Config{},
expectErr: false,
},
{
name: "only CertFile",
config: Config{CertFile: "cert.pem"},
expectErr: true,
},
{
name: "only KeyFile",
config: Config{KeyFile: "key.pem"},
expectErr: true,
},
{
name: "CertFile and KeyFile",
config: Config{CertFile: "cert.pem", KeyFile: "Key.pem"},
expectErr: false,
},
{
name: "CertPem and KeyPem",
config: Config{CertPem: "cert", KeyPem: "key"},
expectErr: false,
},
{
name: "CertFile and KeyPem(mixed)",
config: Config{CertFile: "cert.pem", KeyPem: "key"},
expectErr: false,
},
{
name: "CertPem only",
config: Config{CertPem: "cert"},
expectErr: true,
},
{
name: "KeyPem only",
config: Config{KeyPem: "key"},
expectErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.config.Validate()
if tt.expectErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
})
}
}
8 changes: 8 additions & 0 deletions config/configtls/configtls.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,14 @@ func (c Config) Validate() error {
return errors.New("invalid TLS configuration: min_version cannot be greater than max_version")
}

certProvided := c.CertFile != "" || c.CertPem != ""
keyProvided := c.KeyFile != "" || c.KeyPem != ""

// If cert or key is provided, require both to be present
if certProvided != keyProvided {
return errors.New("TLS configuration must include certificate and key (CertFile/CertPem and KeyFile/KeyPem)")
}
Copy link
Contributor

@jade-guiton-dd jade-guiton-dd Jun 6, 2025

Choose a reason for hiding this comment

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

I don't think this check fixes issue #13130. The issue was originally filed because we noticed the case where the TLS config is empty led to errors, but these errors only surface at connection time. (Admittedly, the issue description may not have been clear about this). But this check doesn't fail if all fields are empty.

Moreover, I believe this is identical to the first check in Config.loadCertificate (c.hasCert() != c.hasKey()), which is run at startup time (slightly later than config validation, but still good enough I would say).

If we want to move as many checks as possible to config validation time, looking at the code for Config.loadCertificate, I think the appropriate check would be c.hasCertFile() != c.hasCertPem() && c.hasKeyFile() != c.hasKeyPem() (ie. we have exactly one field setting a certificate, and exactly one field setting a key).

If we just want to fix the immediate issue, I think checking c.hasCert() || c.hasKey() would be enough.

And if we don't mind an error on startup rather than an error at config validation time, considering the Config.loadCertificate function already HAS a check for this, I think the fix with the smallest diff would be to simply add a proper error message for that case, and remove the if c.hasCert() || c.hasKey() test in Config.loadTLSConfig() which prevents the check from running.


return nil
}

Expand Down
Loading