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

SVIDStore disk plugin #2647

Closed
wants to merge 12 commits into from
9 changes: 9 additions & 0 deletions conf/agent/agent_full.conf
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,15 @@ plugins {
}
}

# SVIDStore "disk": An SVID store that stores the SVIDs on disk
SVIDStore "disk" {
plugin_data {
# directory: Base directory that is used to store the SVIDs.
# All stored files are under this path.
# directory = "/path/to/svids"
}
}

# WorkloadAttestor "docker": A workload attestor which allows selectors
# based on docker constructs such label and image_id.
WorkloadAttestor "docker" {
Expand Down
35 changes: 35 additions & 0 deletions doc/plugin_agent_svidstore_disk.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Agent plugin: SVIDStore "disk"

The `disk` plugin stores in disk the resulting X509-SVIDs of the entries that the agent is entitled to.

### Format

The plugin stores the SVID in three different PEM encoded files: one for the certificate chain, one for the certificate key and one for the trust domain bundle. The file paths are specified through [selectors](#selectors).

_Note: federated bundles are not stored by this plugin._

### Configuration

| Configuration | Description | DEFAULT |
| -------------------- | ----------- | -------------- |
| directory | Base directory that is used to store the SVIDs. All stored files are under this path. | |

A sample configuration:

```
SVIDStore "disk" {
plugin_data {
directory = "/path/to/svids"
}
}
```

### Selectors

Selectors are used on `storable` entries to describre metadata that is needed by the `disk` plugin in order to store the SVIDs on disk. In case that a required selector is not provided, the plugin will return an error at execution time.

| Selector | Example | Required | Description |
| ----------------------------- | ------------------------------------------ | -------- | -------------------------------------------- |
| `disk:certchainfile` | `disk:certchainfile:tls.crt` | x | The file path relative to the base directory where the SVID certificate chain will be stored. |
| `disk:keyfile` | `disk:keyfile:key.crt` | x | The file path relative to the base directory where the SVID certificate key will be stored. |
| `disk:bundlefile` | `disk:bundlefile:ca.crt` | x | The file path relative to the base directory where the CA certificates belonging to the Trust Domain of the SVID will be stored. |
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ require (
go.opencensus.io v0.23.0 // indirect
go.uber.org/atomic v1.9.0
go.uber.org/goleak v1.1.11 // indirect
go.uber.org/multierr v1.6.0 // indirect
go.uber.org/multierr v1.6.0
go.uber.org/zap v1.19.0 // indirect
golang.org/x/crypto v0.0.0-20210915214749-c084706c2272
golang.org/x/net v0.0.0-20210917221730-978cfadd31cf
Expand Down
2 changes: 2 additions & 0 deletions pkg/agent/catalog/svidstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package catalog
import (
"github.com/spiffe/spire/pkg/agent/plugin/svidstore"
"github.com/spiffe/spire/pkg/agent/plugin/svidstore/awssecretsmanager"
"github.com/spiffe/spire/pkg/agent/plugin/svidstore/disk"
"github.com/spiffe/spire/pkg/agent/plugin/svidstore/gcpsecretmanager"
"github.com/spiffe/spire/pkg/common/catalog"
)
Expand All @@ -27,6 +28,7 @@ func (repo *svidStoreRepository) BuiltIns() []catalog.BuiltIn {
return []catalog.BuiltIn{
awssecretsmanager.BuiltIn(),
gcpsecretmanager.BuiltIn(),
disk.BuiltIn(),
}
}

Expand Down
311 changes: 311 additions & 0 deletions pkg/agent/plugin/svidstore/disk/disk.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,311 @@
package disk

import (
"context"
"encoding/pem"
"errors"
"fmt"
"os"
"path/filepath"
"strings"
"sync"

"github.com/hashicorp/go-hclog"
"github.com/hashicorp/hcl"
configv1 "github.com/spiffe/spire-plugin-sdk/proto/spire/service/common/config/v1"
"github.com/spiffe/spire/pkg/agent/plugin/svidstore"
"github.com/spiffe/spire/pkg/common/catalog"
"github.com/spiffe/spire/pkg/common/diskutil"
"github.com/spiffe/spire/pkg/common/telemetry"
svidstorev1 "github.com/spiffe/spire/proto/spire/plugin/agent/svidstore/v1"
"go.uber.org/multierr"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)

const (
pluginName = "disk"
attrName = "user.spire-svid"
)

type certFile struct {
Copy link
Member

Choose a reason for hiding this comment

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

small naming nit: certFile is being used for cert chains, private keys, and bundles. Maybe svidFile?

filePath string
pemBytes []byte
}

type diskStore struct {
certChain certFile
key certFile
bundle certFile
}

func BuiltIn() catalog.BuiltIn {
return builtin(New())
}

func builtin(p *DiskPlugin) catalog.BuiltIn {
return catalog.MakeBuiltIn(pluginName,
svidstorev1.SVIDStorePluginServer(p),
configv1.ConfigServiceServer(p),
)
}

func New() *DiskPlugin {
return &DiskPlugin{}
}

type configuration struct {
Directory string `hcl:"directory" json:"directory"`
}

type DiskPlugin struct {
svidstorev1.UnsafeSVIDStoreServer
configv1.UnsafeConfigServer

log hclog.Logger
config *configuration
trustDomain string
mtx sync.RWMutex
}

// SetLogger sets the logger used by the plugin
func (p *DiskPlugin) SetLogger(log hclog.Logger) {
p.mtx.Lock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

lcoks are not required because it is done in plugin level

defer p.mtx.Unlock()

p.log = log
}

// Configure configures the plugin
func (p *DiskPlugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) (*configv1.ConfigureResponse, error) {
config := &configuration{}
if err := hcl.Decode(config, req.HclConfiguration); err != nil {
return nil, status.Errorf(codes.InvalidArgument, "unable to decode configuration: %v", err)
}

p.mtx.Lock()
defer p.mtx.Unlock()

p.trustDomain = req.CoreConfiguration.TrustDomain
p.config = config
if config.Directory == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: maybe you can move this if above lock just to make it easier to read?

return nil, status.Error(codes.InvalidArgument, "a directory must be configured")
}

return &configv1.ConfigureResponse{}, nil
}

// PutX509SVID stores the specified X509-SVID in the configured location
func (p *DiskPlugin) PutX509SVID(ctx context.Context, req *svidstorev1.PutX509SVIDRequest) (*svidstorev1.PutX509SVIDResponse, error) {
log := p.log.With(telemetry.SPIFFEID, req.Svid.SpiffeID)

config, err := p.getConfig()
if err != nil {
return nil, err
}

diskStore, err := newDiskStore(req.Metadata, config.Directory)
if err != nil {
return nil, status.Error(codes.InvalidArgument, err.Error())
}

diskStore.certChain.pemBytes = certChainPEMBytes(req.Svid.CertChain)
diskStore.key.pemBytes = keyPEMBytes(req.Svid.PrivateKey)
diskStore.bundle.pemBytes = certChainPEMBytes(req.Svid.Bundle)

log.With("cert_chain_file_path", diskStore.certChain.filePath).Debug("Writing certificate chain file")
if err := diskStore.certChain.write(p.trustDomain); err != nil {
return nil, status.Errorf(codes.Internal, "failed to write certificate chain file: %v", err)
}

log.With("key_file_path", diskStore.key.filePath).Debug("Writing key file")
if err := diskStore.key.write(p.trustDomain); err != nil {
return nil, status.Errorf(codes.Internal, "failed to write key file: %v", err)
}

log.With("bundle_file_path", diskStore.bundle.filePath).Debug("Writing bundle file")
if err := diskStore.bundle.write(p.trustDomain); err != nil {
return nil, status.Errorf(codes.Internal, "failed to write bundle file: %v", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to best-effort delete the previously-saved components of the stored SVID if a later write action fails? This would help keep disk usage tidied up

Copy link
Member

Choose a reason for hiding this comment

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

if we are updating a previously stored-SVID, is it possible for us to reach an inconsistent state where the past SVID is partially on disk, and the rest has been overwritten by the updated SVID?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @amoore877 for the feedback. I'm implementing a solution that should address these concerns, providing a reasonable level of atomicity in the write action. I'll be updating the PR with that.


return &svidstorev1.PutX509SVIDResponse{}, nil
}

// DeleteX509SVID deletes the specified stored X509-SVID
func (p *DiskPlugin) DeleteX509SVID(ctx context.Context, req *svidstorev1.DeleteX509SVIDRequest) (*svidstorev1.DeleteX509SVIDResponse, error) {
config, err := p.getConfig()
if err != nil {
return nil, err
}
diskStore, err := newDiskStore(req.Metadata, config.Directory)
if err != nil {
return nil, err
}

if errRemoveCertChain := diskStore.certChain.delete(); errRemoveCertChain != nil {
if os.IsNotExist(errRemoveCertChain) {
p.log.With("file_path", diskStore.certChain.filePath).Warn("Could not delete certificate chain file. File not found")
} else {
err = multierr.Append(err, fmt.Errorf("failed to delete certificate chain file: %w", errRemoveCertChain))
}
}
if errRemoveKey := diskStore.key.delete(); errRemoveKey != nil {
if os.IsNotExist(errRemoveKey) {
p.log.With("file_path", diskStore.key.filePath).Warn("Could not delete key file. File not found")
} else {
err = multierr.Append(err, fmt.Errorf("failed to delete key file: %w", errRemoveKey))
}
}
if errRemoveBundle := diskStore.bundle.delete(); errRemoveBundle != nil {
if os.IsNotExist(errRemoveBundle) {
p.log.With("file_path", diskStore.bundle.filePath).Warn("Could not delete bundle file. File not found")
} else {
err = multierr.Append(err, fmt.Errorf("failed to delete bundle file: %w", errRemoveBundle))
}
}

if err != nil {
return nil, status.Errorf(codes.Internal, "error deleting SVID: %v", err)
}

return &svidstorev1.DeleteX509SVIDResponse{}, nil
}

func (p *DiskPlugin) getConfig() (*configuration, error) {
p.mtx.RLock()
defer p.mtx.RUnlock()
if p.config == nil {
return nil, status.Error(codes.FailedPrecondition, "not configured")
}
return p.config, nil
}

func (c *certFile) write(attrValue string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

functions must be moved together with struct definition

if err := validateXattr(c.filePath, attrValue); err != nil {
return err
}

if err := createBaseDirectoryIfNeeded(c.filePath); err != nil {
return err
}

if err := diskutil.AtomicWriteFile(c.filePath, c.pemBytes, 0600); err != nil {
return err
}
if err := setxattr(c.filePath, attrName, []byte(attrValue)); err != nil {
return fmt.Errorf("failed to set extended attribute to file: %w", err)
}

return nil
}

func (c *certFile) delete() error {
return os.Remove(c.filePath)
}

func getFileMetadata(metadataMap map[string]string, key string) (string, error) {
value := metadataMap[key]
if value == "" {
return "", status.Errorf(codes.InvalidArgument, "%s must be specified", key)
}
if containsDotDot(value) {
return "", status.Errorf(codes.InvalidArgument, "invalid %s", key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it not important return why that key is invalid?

}

return value, nil
}

func newDiskStore(metaData []string, dir string) (*diskStore, error) {
metadataMap, err := svidstore.ParseMetadata(metaData)
if err != nil {
return nil, status.Errorf(codes.InvalidArgument, "error parsing metadata: %v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this function caller your are just getting error message, and creating a new status.
So this status will not be used.

What do you think about, and the same for all another errors form this function, what do you think about in caller just. resturn error instead of getting error message and creating a new status?

Another option is that you can just create regular errors here, but yyou may do the same in all another functions, and it will be a little incosistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

The caller should just return the error as it is, without adding a status. All code paths should return a proper status. I'll fix this.

}

certChainFilePath, err := getFileMetadata(metadataMap, "certchainfile")
if err != nil {
return nil, err
}

keyFilePath, err := getFileMetadata(metadataMap, "keyfile")
if err != nil {
return nil, err
}

bundleFilePath, err := getFileMetadata(metadataMap, "bundlefile")
if err != nil {
return nil, err
}

return &diskStore{
certChain: certFile{filePath: filepath.Join(dir, certChainFilePath)},
key: certFile{filePath: filepath.Join(dir, keyFilePath)},
bundle: certFile{filePath: filepath.Join(dir, bundleFilePath)},
}, nil
}

// validateXattr validates that the specified file has
// an extended attribute (https://en.wikipedia.org/wiki/Extended_file_attributes)
// set by this plugin, with the trust domain as the value.
// This is a best-effort attempt to avoid collisions with other systems.
// Some platforms do not support this mechanism.
func validateXattr(filePath, attrValue string) error {
if _, statErr := os.Stat(filePath); os.IsNotExist(statErr) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if it returns another error? is it not important?

return nil
}

dest := make([]byte, len(attrValue))
err := getxattr(filePath, attrName, dest)
if err != nil {
return fmt.Errorf("validation error: %w", err)
}
if string(dest) != attrValue {
return errors.New("validation error: attribute mismatch")
}

return nil
}

func keyPEMBytes(privateKey []byte) (pemData []byte) {
b := &pem.Block{
Type: "PRIVATE KEY",
Bytes: privateKey,
}

return pem.EncodeToMemory(b)
}

func certChainPEMBytes(certChain [][]byte) (pemData []byte) {
for _, cert := range certChain {
b := &pem.Block{
Type: "CERTIFICATE",
Bytes: cert,
}
pemData = append(pemData, pem.EncodeToMemory(b)...)
}

return pemData
}

func createBaseDirectoryIfNeeded(filePath string) error {
baseDir := filepath.Dir(filePath)
if _, statErr := os.Stat(baseDir); os.IsNotExist(statErr) {
if err := os.MkdirAll(baseDir, 0755); err != nil {
return status.Errorf(codes.Internal, "error creating directory: %v", err)
}
}
return nil
}

func containsDotDot(v string) bool {
if !strings.Contains(v, "..") {
return false
}
for _, ent := range strings.FieldsFunc(v, isSlashRune) {
if ent == ".." {
return true
}
}
return false
}

func isSlashRune(r rune) bool { return r == '/' || r == '\\' }
Loading