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.

Sign up for GitHub

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

Graceful secret removal #1715

Merged
merged 2 commits into from
Nov 4, 2016
Merged

Graceful secret removal #1715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
edb20bc
Reject secrets currently in use
diogomonica Oct 29, 2016
f67b6d7
Added one more case for the denied names blacklist
diogomonica Nov 2, 2016
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
30 changes: 30 additions & 0 deletions manager/controlapi/secret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package controlapi

import (
"regexp"
"strings"

"github.com/Sirupsen/logrus"
"github.com/docker/distribution/digest"
Expand Down Expand Up @@ -191,13 +192,42 @@ func (s *Server) CreateSecret(ctx context.Context, request *api.CreateSecretRequ
// RemoveSecret removes the secret referenced by `RemoveSecretRequest.ID`.
// - Returns `InvalidArgument` if `RemoveSecretRequest.ID` is empty.
// - Returns `NotFound` if the a secret named `RemoveSecretRequest.ID` is not found.
// - Returns `SecretInUse` if the secret is currently in use
// - Returns an error if the deletion fails.
func (s *Server) RemoveSecret(ctx context.Context, request *api.RemoveSecretRequest) (*api.RemoveSecretResponse, error) {
if request.SecretID == "" {
return nil, grpc.Errorf(codes.InvalidArgument, "secret ID must be provided")
}

err := s.store.Update(func(tx store.Tx) error {
// Check if the secret exists
secret := store.GetSecret(tx, request.SecretID)
if secret == nil {
return grpc.Errorf(codes.NotFound, "could not find secret %s", request.SecretID)
}

// Check if any services currently reference this secret, return error if so
services, err := store.FindServices(tx, store.ByReferencedSecretID(request.SecretID))
if err != nil {
return grpc.Errorf(codes.Internal, "could not find services using secret %s: %v", request.SecretID, err)
}

if len(services) != 0 {
serviceNames := make([]string, 0, len(services))
for _, service := range services {
serviceNames = append(serviceNames, service.Spec.Annotations.Name)
}

secretName := secret.Spec.Annotations.Name
serviceNameStr := strings.Join(serviceNames, ", ")
serviceStr := "services"
if len(serviceNames) == 1 {
serviceStr = "service"
}

return grpc.Errorf(codes.InvalidArgument, "secret '%s' is in use by the following %s: %v", secretName, serviceStr, serviceNameStr)
}

return store.DeleteSecret(tx, request.SecretID)
})
switch err {
Expand Down
103 changes: 79 additions & 24 deletions manager/controlapi/secret_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,11 @@ func TestValidateSecretSpec(t *testing.T) {
}

func TestCreateSecret(t *testing.T) {
s := newTestServer(t)
ts := newTestServer(t)
defer ts.Stop()

// ---- creating a secret with an invalid spec fails, thus checking that CreateSecret validates the spec ----
_, err := s.Client.CreateSecret(context.Background(), &api.CreateSecretRequest{Spec: createSecretSpec("", nil, nil)})
_, err := ts.Client.CreateSecret(context.Background(), &api.CreateSecretRequest{Spec: createSecretSpec("", nil, nil)})
assert.Error(t, err)
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))

Expand All @@ -113,7 +114,7 @@ func TestCreateSecret(t *testing.T) {
creationSpec := createSecretSpec("name", data, nil)
validSpecRequest := api.CreateSecretRequest{Spec: creationSpec}

resp, err := s.Client.CreateSecret(context.Background(), &validSpecRequest)
resp, err := ts.Client.CreateSecret(context.Background(), &validSpecRequest)
assert.NoError(t, err)
assert.NotNil(t, resp)
assert.NotNil(t, resp.Secret)
Expand All @@ -124,39 +125,40 @@ func TestCreateSecret(t *testing.T) {

// for sanity, check that the stored secret still has the secret data
var storedSecret *api.Secret
s.Store.View(func(tx store.ReadTx) {
ts.Store.View(func(tx store.ReadTx) {
storedSecret = store.GetSecret(tx, resp.Secret.ID)
})
assert.NotNil(t, storedSecret)
assert.Equal(t, data, storedSecret.Spec.Data)

// ---- creating a secret with the same name, even if it's the exact same spec, fails due to a name conflict ----
_, err = s.Client.CreateSecret(context.Background(), &validSpecRequest)
_, err = ts.Client.CreateSecret(context.Background(), &validSpecRequest)
assert.Error(t, err)
assert.Equal(t, codes.AlreadyExists, grpc.Code(err), grpc.ErrorDesc(err))
}

func TestGetSecret(t *testing.T) {
s := newTestServer(t)
ts := newTestServer(t)
defer ts.Stop()

// ---- getting a secret without providing an ID results in an InvalidArgument ----
_, err := s.Client.GetSecret(context.Background(), &api.GetSecretRequest{})
_, err := ts.Client.GetSecret(context.Background(), &api.GetSecretRequest{})
assert.Error(t, err)
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))

// ---- getting a non-existent secret fails with NotFound ----
_, err = s.Client.GetSecret(context.Background(), &api.GetSecretRequest{SecretID: "12345"})
_, err = ts.Client.GetSecret(context.Background(), &api.GetSecretRequest{SecretID: "12345"})
assert.Error(t, err)
assert.Equal(t, codes.NotFound, grpc.Code(err), grpc.ErrorDesc(err))

// ---- getting an existing secret returns the secret with all the private data cleaned ----
secret := secretFromSecretSpec(createSecretSpec("name", []byte("data"), nil))
err = s.Store.Update(func(tx store.Tx) error {
err = ts.Store.Update(func(tx store.Tx) error {
return store.CreateSecret(tx, secret)
})
assert.NoError(t, err)

resp, err := s.Client.GetSecret(context.Background(), &api.GetSecretRequest{SecretID: secret.ID})
resp, err := ts.Client.GetSecret(context.Background(), &api.GetSecretRequest{SecretID: secret.ID})
assert.NoError(t, err)
assert.NotNil(t, resp)
assert.NotNil(t, resp.Secret)
Expand All @@ -168,29 +170,31 @@ func TestGetSecret(t *testing.T) {
}

func TestUpdateSecret(t *testing.T) {
s := newTestServer(t)
ts := newTestServer(t)
defer ts.Stop()

// Add a secret to the store to update
secret := secretFromSecretSpec(createSecretSpec("name", []byte("data"), map[string]string{"mod2": "0", "mod4": "0"}))
err := s.Store.Update(func(tx store.Tx) error {
err := ts.Store.Update(func(tx store.Tx) error {
return store.CreateSecret(tx, secret)
})
assert.NoError(t, err)

// ---- updating a secret without providing an ID results in an InvalidArgument ----
_, err = s.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{})
_, err = ts.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{})
assert.Error(t, err)
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))

// ---- getting a non-existent secret fails with NotFound ----
_, err = s.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{SecretID: "1234adsaa", SecretVersion: &api.Version{Index: 1}})
_, err = ts.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{SecretID: "1234adsaa", SecretVersion: &api.Version{Index: 1}})
assert.Error(t, err)
assert.Equal(t, codes.NotFound, grpc.Code(err), grpc.ErrorDesc(err))

// ---- updating an existing secret's labels returns the secret with all the private data cleaned ----
newLabels := map[string]string{"mod2": "0", "mod4": "0", "mod6": "0"}
secret.Spec.Annotations.Labels = newLabels
secret.Spec.Data = nil
resp, err := s.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{
resp, err := ts.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{
SecretID: secret.ID,
Spec: &secret.Spec,
SecretVersion: &secret.Meta.Version,
Expand All @@ -203,40 +207,91 @@ func TestUpdateSecret(t *testing.T) {

// ---- updating an existing secret's data returns an error ----
secret.Spec.Data = []byte{1}
resp, err = s.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{
resp, err = ts.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{
SecretID: secret.ID,
Spec: &secret.Spec,
SecretVersion: &secret.Meta.Version,
})
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))
}

func TestRemoveSecret(t *testing.T) {
s := newTestServer(t)
func TestRemoveUnusedSecret(t *testing.T) {
ts := newTestServer(t)
defer ts.Stop()

// ---- removing a secret without providing an ID results in an InvalidArgument ----
_, err := s.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{})
// removing a secret without providing an ID results in an InvalidArgument
_, err := ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{})
assert.Error(t, err)
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))

// ---- removing a secret that exists succeeds ----
// removing a secret that exists succeeds
secret := secretFromSecretSpec(createSecretSpec("name", []byte("data"), nil))
err = s.Store.Update(func(tx store.Tx) error {
err = ts.Store.Update(func(tx store.Tx) error {
return store.CreateSecret(tx, secret)
})
assert.NoError(t, err)

resp, err := s.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: secret.ID})
resp, err := ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: secret.ID})
assert.NoError(t, err)
assert.Equal(t, api.RemoveSecretResponse{}, *resp)

// ---- it was really removed because attempting to remove it again fails with a NotFound ----
_, err = s.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: secret.ID})
_, err = ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: secret.ID})
assert.Error(t, err)
assert.Equal(t, codes.NotFound, grpc.Code(err), grpc.ErrorDesc(err))

}

func TestRemoveUsedSecret(t *testing.T) {
ts := newTestServer(t)
defer ts.Stop()

// Create two secrets
data := []byte("secret")
creationSpec := createSecretSpec("secretID1", data, nil)
resp, err := ts.Client.CreateSecret(context.Background(), &api.CreateSecretRequest{Spec: creationSpec})
assert.NoError(t, err)
creationSpec2 := createSecretSpec("secretID2", data, nil)
resp2, err := ts.Client.CreateSecret(context.Background(), &api.CreateSecretRequest{Spec: creationSpec2})
assert.NoError(t, err)

// Create a service that uses a secret
service := createSpec("service1", "image", 1)
secretRefs := []*api.SecretReference{
{
SecretName: resp.Secret.Spec.Annotations.Name,
SecretID: resp.Secret.ID,
Target: &api.SecretReference_File{
File: &api.SecretReference_FileTarget{
Name: "target.txt",
},
},
},
}
service.Task.GetContainer().Secrets = secretRefs
_, err = ts.Client.CreateService(context.Background(), &api.CreateServiceRequest{Spec: service})
assert.NoError(t, err)

service2 := createSpec("service2", "image", 1)
service2.Task.GetContainer().Secrets = secretRefs
_, err = ts.Client.CreateService(context.Background(), &api.CreateServiceRequest{Spec: service2})
assert.NoError(t, err)

// removing a secret that exists but is in use fails
_, err = ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: resp.Secret.ID})
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))
assert.Regexp(t, "service[1-2], service[1-2]", grpc.ErrorDesc(err))

// removing a secret that exists but is not in use succeeds
_, err = ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: resp2.Secret.ID})
assert.NoError(t, err)

// it was really removed because attempting to remove it again fails with a NotFound
_, err = ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: resp2.Secret.ID})
assert.Error(t, err)
assert.Equal(t, codes.NotFound, grpc.Code(err), grpc.ErrorDesc(err))
}

func TestListSecrets(t *testing.T) {
s := newTestServer(t)

Expand Down
2 changes: 1 addition & 1 deletion manager/controlapi/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ func TestSecretValidation(t *testing.T) {
assert.NoError(t, err)

// test secret References with invalid filenames
invalidFileNames := []string{"../secretfile.txt", "../../secretfile.txt", "file../.txt"}
invalidFileNames := []string{"../secretfile.txt", "../../secretfile.txt", "file../.txt", "subdir/file.txt"}
for i, invalidName := range invalidFileNames {
secretRef := createSecret(t, ts, invalidName, invalidName)

Expand Down