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

fix(windows-agent): Landscape TLS by default #995

Merged
merged 4 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func newConnection(ctx context.Context, d serviceData) (conn *connection, err er
cancel: cancel,
}

creds, err := transportCredentials(conn.settings.certificatePath)
creds, err := transportCredentials(ctx, conn.settings.certificatePath)
if err != nil {
return nil, err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,8 @@ func testReceiveCommand(t *testing.T, distrosettings distroSettings, homedir str
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)

ctx = context.WithValue(ctx, landscape.InsecureCredentials, true)

tb.wslMock = wslmock.New()
ctx = wsl.WithMock(ctx, tb.wslMock)

Expand Down
28 changes: 18 additions & 10 deletions windows-agent/internal/proservices/landscape/landscape_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ func TestConnect(t *testing.T) {
emptyToken bool
tokenErr bool

requireCertificate bool
clientUsesTLS bool
serverUsesTLS bool
breakLandscapeClientConfig bool

breakUIDFile bool
Expand All @@ -123,7 +124,7 @@ func TestConnect(t *testing.T) {
}{
"Success": {},
"Success in non-first contact": {uid: "123", wantSingleMessage: true},
"Success with an SSL certificate": {requireCertificate: true},
"Success with an SSL certificate": {clientUsesTLS: true, serverUsesTLS: true},

// These tests are for the error cases when the error is logged but not returned
"Silent error when the config is empty": {wantNotConnected: true},
Expand All @@ -138,25 +139,30 @@ func TestConnect(t *testing.T) {
"Error when the first-contact SendUpdatedInfo fails": {tokenErr: true, wantErr: true},
"Error when the config cannot be accessed": {breakLandscapeClientConfig: true, wantErr: true},
"Error when the config cannot be parsed": {wantErr: true},
"Error when the SSL certificate cannot be read": {wantErr: true},
"Error when the SSL certificate is not valid": {wantErr: true},
"Error when the SSL certificate cannot be read": {clientUsesTLS: true, serverUsesTLS: true, wantErr: true},
"Error when the SSL certificate is not valid": {clientUsesTLS: true, serverUsesTLS: true, wantErr: true},
"Error when the SSL certificate is not trusted": {clientUsesTLS: true, serverUsesTLS: true, wantErr: true},
}

for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

if wsl.MockAvailable() {
t.Parallel()
ctx = wsl.WithMock(ctx, wslmock.New())
if !tc.clientUsesTLS {
ctx = context.WithValue(ctx, landscape.InsecureCredentials, true)
}

p := ""
if tc.requireCertificate {
if tc.serverUsesTLS {
p = certPath
}

if wsl.MockAvailable() {
t.Parallel()
ctx = wsl.WithMock(ctx, wslmock.New())
}

lis, server, mockService := setUpLandscapeMock(t, ctx, "localhost:", p)
defer lis.Close()

Expand Down Expand Up @@ -305,7 +311,7 @@ func TestSendUpdatedInfo(t *testing.T) {

for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
ctx := context.Background()
ctx := context.WithValue(context.Background(), landscape.InsecureCredentials, true)
if wsl.MockAvailable() {
t.Parallel()
mock := wslmock.New()
Expand Down Expand Up @@ -519,7 +525,7 @@ func TestAutoReconnection(t *testing.T) {

for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
ctx, cancel := context.WithCancel(context.WithValue(context.Background(), landscape.InsecureCredentials, true))
defer cancel()

if wsl.MockAvailable() {
Expand Down Expand Up @@ -760,6 +766,8 @@ func TestReconnect(t *testing.T) {
certPath = t.TempDir()
testutils.GenerateTempCertificate(t, certPath)
lcapeConfig = fmt.Sprintf("%s\nssl_public_key = {{ .CertPath }}/cert.pem", defaultLandscapeConfig)
} else {
ctx = context.WithValue(ctx, landscape.InsecureCredentials, true)
}

lis, server, mockServerService := setUpLandscapeMock(t, ctx, "localhost:", certPath)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[client]

[host]
url = {{ .HostURL }}
CarlosNihelton marked this conversation as resolved.
Show resolved Hide resolved
33 changes: 28 additions & 5 deletions windows-agent/internal/proservices/landscape/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,27 +100,50 @@
return info, nil
}

type transportCredentialsType struct{}

// InsecureCredentials is the key used in tests for insecure credentials.
var InsecureCredentials = transportCredentialsType{}

// transportCredentials reads the Landscape client config to check if a SSL public key is specified.
//
// If this credential is not specified, an insecure credential is returned.
// If the credential is specified but erroneous, an error is returned.
func transportCredentials(sslPublicKeyPath string) (cred credentials.TransportCredentials, err error) {
// If this credential is not specified, credentials based on the system's certificate pool is returned.
// If the SSL public key is specified but invalid, an error is returned.
// If the context has the "InsecureCredentials" key set to "true", insecure credentials are returned (for testing purposes).
func transportCredentials(ctx context.Context, sslPublicKeyPath string) (cred credentials.TransportCredentials, err error) {
defer decorate.OnError(&err, "Landscape credentials")

if sslPublicKeyPath == "" {
isInsecure := ctx.Value(InsecureCredentials)
// ctx.Value() returns 'any', thus this comparison is cleaner than a type assertion.
if isInsecure == true {
log.Warningf(ctx, "Landscape: context requires insecure credentials, ignoring server's public key %s", sslPublicKeyPath)
return insecure.NewCredentials(), nil
}

if sslPublicKeyPath == "" {
certPool, err := x509.SystemCertPool()
if err != nil {
return nil, fmt.Errorf("could not load system certificates: %v", err)
}

Check warning on line 127 in windows-agent/internal/proservices/landscape/utils.go

View check run for this annotation

Codecov / codecov/patch

windows-agent/internal/proservices/landscape/utils.go#L126-L127

Added lines #L126 - L127 were not covered by tests

return credentials.NewTLS(&tls.Config{
RootCAs: certPool,
MinVersion: tls.VersionTLS12,
}), nil
}

log.Infof(ctx, "Landscape: loading server's SSL public key %s", sslPublicKeyPath)
cert, err := os.ReadFile(sslPublicKeyPath)
if err != nil {
return nil, fmt.Errorf("could not load SSL public key file: %v", err)
}

certPool := x509.NewCertPool()
if ok := certPool.AppendCertsFromPEM(cert); !ok {
return nil, fmt.Errorf("failed to add server CA's certificate: %v", err)
return nil, fmt.Errorf("failed to add server's certificate to the trust pool: %v", err)
}

log.Infof(ctx, "Landscape: using server's SSL public key %s instead of system's certificate pool", sslPublicKeyPath)
return credentials.NewTLS(&tls.Config{
RootCAs: certPool,
MinVersion: tls.VersionTLS12,
Expand Down
Loading