Skip to content

Commit ba02a58

Browse files
DevipriyaS17DevipriyaS17
authored
fix: handle encryption and decryption errors instead of silently ignoring them (#778)
* fix: handle password encryption errors in dtoToEntity functions * fix: handle errors in decrypt and fix UT
1 parent e16afa8 commit ba02a58

12 files changed

Lines changed: 127 additions & 67 deletions

File tree

‎internal/mocks/devicemanagement_mocks.go‎

Lines changed: 3 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/usecase/amtexplorer/wsman.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,12 @@ func (g GoWSMANMessages) SetupWsmanClient(device entity.Device, logAMTMessages b
5454
clientParams.PinnedCert = *device.CertHash
5555
}
5656

57-
clientParams.Password, _ = g.safeRequirements.Decrypt(device.Password)
57+
decryptedPassword, err := g.safeRequirements.Decrypt(device.Password)
58+
if err != nil {
59+
return nil, err
60+
}
61+
62+
clientParams.Password = decryptedPassword
5863

5964
connectionsMu.Lock()
6065
defer connectionsMu.Unlock()

‎internal/usecase/ciraconfigs/usecase.go‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,10 @@ func (uc *UseCase) Delete(ctx context.Context, configName, tenantID string) erro
8989
}
9090

9191
func (uc *UseCase) Update(ctx context.Context, d *dto.CIRAConfig) (*dto.CIRAConfig, error) {
92-
d1 := uc.dtoToEntity(d)
92+
d1, err := uc.dtoToEntity(d)
93+
if err != nil {
94+
return nil, err
95+
}
9396

9497
updated, err := uc.repo.Update(ctx, d1)
9598
if err != nil {
@@ -111,9 +114,12 @@ func (uc *UseCase) Update(ctx context.Context, d *dto.CIRAConfig) (*dto.CIRAConf
111114
}
112115

113116
func (uc *UseCase) Insert(ctx context.Context, d *dto.CIRAConfig) (*dto.CIRAConfig, error) {
114-
d1 := uc.dtoToEntity(d)
117+
d1, err := uc.dtoToEntity(d)
118+
if err != nil {
119+
return nil, err
120+
}
115121

116-
_, err := uc.repo.Insert(ctx, d1)
122+
_, err = uc.repo.Insert(ctx, d1)
117123
if err != nil {
118124
return nil, ErrDatabase.Wrap("Insert", "uc.repo.Insert", err)
119125
}
@@ -129,7 +135,7 @@ func (uc *UseCase) Insert(ctx context.Context, d *dto.CIRAConfig) (*dto.CIRAConf
129135
}
130136

131137
// convert dto.CIRAConfig to entity.CIRAConfig.
132-
func (uc *UseCase) dtoToEntity(d *dto.CIRAConfig) *entity.CIRAConfig {
138+
func (uc *UseCase) dtoToEntity(d *dto.CIRAConfig) (*entity.CIRAConfig, error) {
133139
d1 := &entity.CIRAConfig{
134140
ConfigName: d.ConfigName,
135141
MPSAddress: d.MPSAddress,
@@ -145,10 +151,15 @@ func (uc *UseCase) dtoToEntity(d *dto.CIRAConfig) *entity.CIRAConfig {
145151
GenerateRandomPassword: d.GenerateRandomPassword,
146152
Version: d.Version,
147153
}
154+
155+
var err error
148156
// Encrypt password before storing
149-
d1.Password, _ = uc.safeRequirements.Encrypt(d.Password)
157+
d1.Password, err = uc.safeRequirements.Encrypt(d.Password)
158+
if err != nil {
159+
return nil, ErrCIRAConfigsUseCase.Wrap("dtoToEntity", "failed to encrypt password", err)
160+
}
150161

151-
return d1
162+
return d1, nil
152163
}
153164

154165
// convert entity.CIRAConfig to dto.CIRAConfig.

‎internal/usecase/devices/interceptor.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,9 +111,17 @@ func (uc *UseCase) getOrCreateConnection(c context.Context, conn *websocket.Conn
111111
}
112112

113113
func (uc *UseCase) createNewConnection(c context.Context, conn *websocket.Conn, key string, device *entity.Device) (*DeviceConnection, error) {
114-
wsmanConnection := uc.redirection.SetupWsmanClient(*device, true, true)
114+
wsmanConnection, err := uc.redirection.SetupWsmanClient(*device, true, true)
115+
if err != nil {
116+
return nil, err
117+
}
118+
119+
decryptedPassword, err := uc.safeRequirements.Decrypt(device.Password)
120+
if err != nil {
121+
return nil, err
122+
}
115123

116-
device.Password, _ = uc.safeRequirements.Decrypt(device.Password)
124+
device.Password = decryptedPassword
117125

118126
ctx, cancel := context.WithCancel(c)
119127
now := time.Now()

‎internal/usecase/devices/interceptor_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ func TestRedirect(t *testing.T) {
6666
Username: "user",
6767
Password: "pass",
6868
}, nil)
69-
mockRedir.EXPECT().SetupWsmanClient(gomock.Any(), true, true).Return(wsman.Messages{})
69+
mockRedir.EXPECT().SetupWsmanClient(gomock.Any(), true, true).Return(wsman.Messages{}, nil)
7070
mockRedir.EXPECT().RedirectConnect(gomock.Any(), gomock.Any()).Return(ErrInterceptorGeneral)
7171
},
7272
expectedErr: ErrInterceptorGeneral,
@@ -138,7 +138,7 @@ func TestRedirectSuccessfulFlow(t *testing.T) {
138138

139139
// Mock successful flow up to RedirectConnect, then fail to avoid goroutines
140140
mockRepo.EXPECT().GetByID(gomock.Any(), testGUID, "").Return(device, nil)
141-
mockRedirection.EXPECT().SetupWsmanClient(*device, true, true).Return(wsman.Messages{})
141+
mockRedirection.EXPECT().SetupWsmanClient(*device, true, true).Return(wsman.Messages{}, nil)
142142
// Return error to avoid starting problematic goroutines but still test the flow
143143
mockRedirection.EXPECT().RedirectConnect(gomock.Any(), gomock.Any()).Return(ErrConnectionFailed)
144144

@@ -211,15 +211,15 @@ func TestRedirectConnectionReuse(t *testing.T) {
211211

212212
// First call - create new connection but fail at connect to avoid goroutines
213213
mockRepo.EXPECT().GetByID(gomock.Any(), testGUID, "").Return(device, nil)
214-
mockRedirection.EXPECT().SetupWsmanClient(*device, true, true).Return(wsman.Messages{})
214+
mockRedirection.EXPECT().SetupWsmanClient(*device, true, true).Return(wsman.Messages{}, nil)
215215
mockRedirection.EXPECT().RedirectConnect(gomock.Any(), gomock.Any()).Return(ErrFirstConnectionFailed)
216216

217217
err := uc.Redirect(context.Background(), mockConn, testGUID, testMode)
218218
require.Error(t, err)
219219

220220
// Second call - also fail to avoid goroutines but test reuse logic
221221
mockRepo.EXPECT().GetByID(gomock.Any(), testGUID, "").Return(device, nil)
222-
mockRedirection.EXPECT().SetupWsmanClient(*device, true, true).Return(wsman.Messages{})
222+
mockRedirection.EXPECT().SetupWsmanClient(*device, true, true).Return(wsman.Messages{}, nil)
223223
mockRedirection.EXPECT().RedirectConnect(gomock.Any(), gomock.Any()).Return(ErrSecondConnectionFailed)
224224

225225
err = uc.Redirect(context.Background(), mockConn, testGUID, testMode)
@@ -305,7 +305,7 @@ func TestRedirectWithErrorScenarios(t *testing.T) {
305305

306306
device := &entity.Device{GUID: testGUID, Username: "user", Password: "pass"}
307307
mockRepo.EXPECT().GetByID(gomock.Any(), testGUID, "").Return(device, nil)
308-
mockRedir.EXPECT().SetupWsmanClient(*device, true, true).Return(wsman.Messages{})
308+
mockRedir.EXPECT().SetupWsmanClient(*device, true, true).Return(wsman.Messages{}, nil)
309309
mockRedir.EXPECT().RedirectConnect(gomock.Any(), gomock.Any()).Return(ErrConnectionFailed)
310310
},
311311
expectedErr: "connection failed",
@@ -383,7 +383,7 @@ func TestRedirectConnectionFlowCoverage(t *testing.T) {
383383

384384
device := &entity.Device{GUID: "test-device", Username: "user", Password: "pass"}
385385
mockRepo.EXPECT().GetByID(gomock.Any(), "test-device", "").Return(device, nil)
386-
mockRedir.EXPECT().SetupWsmanClient(*device, true, true).Return(wsman.Messages{})
386+
mockRedir.EXPECT().SetupWsmanClient(*device, true, true).Return(wsman.Messages{}, nil)
387387
// Return error to avoid starting goroutines, but still exercise connection creation
388388
mockRedir.EXPECT().RedirectConnect(gomock.Any(), gomock.Any()).Return(ErrTestError)
389389
},

‎internal/usecase/devices/interfaces.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ type (
2828
}
2929

3030
Redirection interface {
31-
SetupWsmanClient(device entity.Device, isRedirection, logMessages bool) wsman.Messages
31+
SetupWsmanClient(device entity.Device, isRedirection, logMessages bool) (wsman.Messages, error)
3232
RedirectConnect(ctx context.Context, deviceConnection *DeviceConnection) error
3333
RedirectClose(ctx context.Context, deviceConnection *DeviceConnection) error
3434
RedirectListen(ctx context.Context, deviceConnection *DeviceConnection) ([]byte, error)

‎internal/usecase/devices/redirection.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ type Redirector struct {
1414
SafeRequirements security.Cryptor
1515
}
1616

17-
func (g *Redirector) SetupWsmanClient(device entity.Device, isRedirection, logAMTMessages bool) wsman.Messages {
17+
func (g *Redirector) SetupWsmanClient(device entity.Device, isRedirection, logAMTMessages bool) (wsman.Messages, error) {
1818
clientParams := client.Parameters{
1919
Target: device.Hostname,
2020
Username: device.Username,
@@ -29,9 +29,14 @@ func (g *Redirector) SetupWsmanClient(device entity.Device, isRedirection, logAM
2929
clientParams.PinnedCert = *device.CertHash
3030
}
3131

32-
clientParams.Password, _ = g.SafeRequirements.Decrypt(device.Password)
32+
decryptedPassword, err := g.SafeRequirements.Decrypt(device.Password)
33+
if err != nil {
34+
return wsman.Messages{}, err
35+
}
36+
37+
clientParams.Password = decryptedPassword
3338

34-
return wsman.NewMessages(clientParams)
39+
return wsman.NewMessages(clientParams), nil
3540
}
3641

3742
func NewRedirector(safeRequirements security.Cryptor) *Redirector {

‎internal/usecase/devices/redirection_test.go‎

Lines changed: 9 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,9 @@ func initRedirectionTest(t *testing.T) (*devices.Redirector, *mocks.MockRedirect
2828
}
2929

3030
type redTest struct {
31-
name string
32-
redMock func(*mocks.MockRedirection)
33-
res any
31+
name string
32+
res any
33+
err error
3434
}
3535

3636
func TestSetupWsmanClient(t *testing.T) {
@@ -44,21 +44,8 @@ func TestSetupWsmanClient(t *testing.T) {
4444
tests := []redTest{
4545
{
4646
name: "success",
47-
redMock: func(redirect *mocks.MockRedirection) {
48-
redirect.EXPECT().
49-
SetupWsmanClient(gomock.Any(), false, true).
50-
Return(wsman.Messages{})
51-
},
52-
res: wsman.Messages{},
53-
},
54-
{
55-
name: "fail",
56-
redMock: func(redirect *mocks.MockRedirection) {
57-
redirect.EXPECT().
58-
SetupWsmanClient(gomock.Any(), true, true).
59-
Return(wsman.Messages{})
60-
},
61-
res: wsman.Messages{},
47+
res: wsman.Messages{},
48+
err: nil,
6249
},
6350
}
6451

@@ -67,17 +54,14 @@ func TestSetupWsmanClient(t *testing.T) {
6754
t.Run(tc.name, func(t *testing.T) {
6855
t.Parallel()
6956

70-
redirector, redirect, _ := initRedirectionTest(t)
71-
72-
tc.redMock(redirect)
57+
redirector, _, _ := initRedirectionTest(t)
7358

74-
redirector.SafeRequirements = security.Crypto{
75-
EncryptionKey: "test",
76-
}
59+
redirector.SafeRequirements = mocks.MockCrypto{}
7760

78-
res := redirector.SetupWsmanClient(*device, true, true)
61+
res, err := redirector.SetupWsmanClient(*device, true, true)
7962

8063
require.IsType(t, tc.res, res)
64+
require.Equal(t, tc.err, err)
8165
})
8266
}
8367
}

‎internal/usecase/devices/wsman/message.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,15 @@ func (g GoWSMANMessages) SetupWsmanClient(device entity.Device, isRedirection, l
131131
errChan := make(chan error, 1)
132132
// Queue the request
133133
requestQueue <- func() {
134-
device.Password, _ = g.safeRequirements.Decrypt(device.Password)
134+
decryptedPassword, err := g.safeRequirements.Decrypt(device.Password)
135+
if err != nil {
136+
errChan <- err
137+
138+
return
139+
}
140+
141+
device.Password = decryptedPassword
142+
135143
if device.MPSUsername != "" {
136144
if len(Connections) == 0 {
137145
errChan <- ErrCIRADeviceNotConnected

‎internal/usecase/domains/usecase.go‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,10 @@ func (uc *UseCase) Delete(ctx context.Context, domainName, tenantID string) erro
172172
}
173173

174174
func (uc *UseCase) Update(ctx context.Context, d *dto.Domain) (*dto.Domain, error) {
175-
d1 := uc.dtoToEntity(d)
175+
d1, err := uc.dtoToEntity(d)
176+
if err != nil {
177+
return nil, err
178+
}
176179

177180
updated, err := uc.repo.Update(ctx, d1)
178181
if err != nil {
@@ -199,7 +202,11 @@ func (uc *UseCase) Insert(ctx context.Context, d *dto.Domain) (*dto.Domain, erro
199202
return nil, err
200203
}
201204

202-
d1 := uc.dtoToEntity(d)
205+
d1, err := uc.dtoToEntity(d)
206+
if err != nil {
207+
return nil, err
208+
}
209+
203210
d1.ExpirationDate = cert.NotAfter.Format(time.RFC3339)
204211

205212
// Store certificate in Vault (if available) - cert goes to Vault, not DB
@@ -267,7 +274,7 @@ func DecryptAndCheckCertExpiration(domain dto.Domain) (*x509.Certificate, error)
267274
}
268275

269276
// convert dto.Domain to entity.Domain.
270-
func (uc *UseCase) dtoToEntity(d *dto.Domain) *entity.Domain {
277+
func (uc *UseCase) dtoToEntity(d *dto.Domain) (*entity.Domain, error) {
271278
d1 := &entity.Domain{
272279
ProfileName: d.ProfileName,
273280
DomainSuffix: d.DomainSuffix,
@@ -278,9 +285,14 @@ func (uc *UseCase) dtoToEntity(d *dto.Domain) *entity.Domain {
278285
Version: d.Version,
279286
}
280287

281-
d1.ProvisioningCertPassword, _ = uc.safeRequirements.Encrypt(d.ProvisioningCertPassword)
288+
var err error
282289

283-
return d1
290+
d1.ProvisioningCertPassword, err = uc.safeRequirements.Encrypt(d.ProvisioningCertPassword)
291+
if err != nil {
292+
return nil, ErrDomainsUseCase.Wrap("dtoToEntity", "failed to encrypt provisioning cert password", err)
293+
}
294+
295+
return d1, nil
284296
}
285297

286298
// convert entity.Domain to dto.Domain.

0 commit comments

Comments
 (0)