mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-20 21:59:07 +02:00
[management] Check a record when TLS verification is switched back on
The skip-TLS exemption added a hole of its own. Such a record is stored without being checked at all, and the re-check on update watched only the upstream, the key and the catalog entry — so turning verification back on left a provider that had never been checked now running as if it had. That transition is the first moment the record can be checked, and it now is. An update that preserves the stored key trims it on the way through, so a record saved before keys were normalised is repaired by the next edit rather than carrying whitespace the proxy still sends. That makes the comparison see a change, which is right: the key has never been tested in the form it is about to be sent in. The create description also claimed more of the upstream than the check reads. Only the host is used — the listing goes to the catalog's own path over HTTPS — so a configured scheme or path is neither used nor validated there.
This commit is contained in:
@@ -576,3 +576,30 @@ func TestCreateProvider_TheStoredKeyIsTheOneThatWasChecked(t *testing.T) {
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "sk-good", stored.APIKey, "and that is the one the proxy will send")
|
||||
}
|
||||
|
||||
// TestUpdateProvider_TurningTlsVerificationBackOnChecksTheRecord covers the
|
||||
// hole the skip-TLS exemption opens on its own. Such a record is stored without
|
||||
// ever being checked, so the moment verification is switched back on is the
|
||||
// first moment it can be checked at all — and none of the three fields the
|
||||
// re-check usually watches has to move for that to happen.
|
||||
func TestUpdateProvider_TurningTlsVerificationBackOnChecksTheRecord(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
f := newBootstrapFixture(t)
|
||||
f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true)
|
||||
|
||||
unchecked := newCheckedProvider("account1")
|
||||
unchecked.SkipTLSVerification = true
|
||||
created, err := f.manager.CreateProvider(ctx, "user1", unchecked)
|
||||
require.NoError(t, err)
|
||||
require.Zero(t, f.vendor.calls(), "the create was exempt")
|
||||
|
||||
f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Update, true)
|
||||
edit := newCheckedProvider("account1")
|
||||
edit.ID = created.ID
|
||||
edit.APIKey = ""
|
||||
edit.SkipTLSVerification = false
|
||||
|
||||
_, err = f.manager.UpdateProvider(ctx, "user1", edit)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, 1, f.vendor.calls(), "switching verification on must check what was never checked")
|
||||
}
|
||||
|
||||
@@ -318,7 +318,12 @@ func (m *managerImpl) UpdateProvider(ctx context.Context, userID string, provide
|
||||
// real key, but it must not silently overwrite a valid stored key.
|
||||
switch trimmed := strings.TrimSpace(provider.APIKey); {
|
||||
case provider.APIKey == "":
|
||||
provider.APIKey = existing.APIKey
|
||||
// Trimmed on the way through: a record stored before keys were
|
||||
// normalised carries whitespace the proxy still sends, and an edit
|
||||
// that preserves the key is the occasion to repair it. Doing so makes
|
||||
// the comparison below see a change, which is correct — that key has
|
||||
// never been tested in the form it is about to be sent in.
|
||||
provider.APIKey = strings.TrimSpace(existing.APIKey)
|
||||
case trimmed == "":
|
||||
return nil, status.Errorf(status.InvalidArgument, "api_key must be non-blank when rotating an agent network provider")
|
||||
default:
|
||||
@@ -338,9 +343,15 @@ func (m *managerImpl) UpdateProvider(ctx context.Context, userID string, provide
|
||||
// The comparison runs after the merge above, so an update that changes only
|
||||
// the URL reads as unchanged on the key and is checked against the stored
|
||||
// one, which is the only credential the operator has to offer here.
|
||||
//
|
||||
// Turning TLS verification back on is the fourth: the record was stored
|
||||
// unchecked precisely because that flag was set, so this is the first
|
||||
// moment it can be checked at all, and nothing else about it need change
|
||||
// for that to be true.
|
||||
if provider.UpstreamURL != existing.UpstreamURL ||
|
||||
provider.APIKey != existing.APIKey ||
|
||||
provider.ProviderID != existing.ProviderID {
|
||||
provider.ProviderID != existing.ProviderID ||
|
||||
(existing.SkipTLSVerification && !provider.SkipTLSVerification) {
|
||||
if err := m.checkProviderCredential(ctx, provider); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user