SVIDStore `disk` plugin #2647

amartinezfayo · 2021-12-03T00:55:03Z

Implementation of the SVIDStore disk plugin.

Fixes #2077.

Signed-off-by: Agustín Martínez Fayó <[email protected]>

loveyana · 2021-12-03T11:05:32Z

hi @amartinezfayo , I am glad that a local disk-based svid component has been almost completed.

I compiled this change into a plug-in separately and tested it in the local and docker environment.
I found that setxattr only supports setxattr in our debian 9 version For the attribute creation of the specified namespace, https://man7.org/linux/man-pages/man5/attr.5.html ,but the constant spire-svid is used as the attribute name in your plug-in, do you mind modifying this problem, regard it as a configuration or modify it as a user namespace attribute, I think it is a good idea, below Is a screenshot of the execution in our environment.

this is the result of local execution, and the execution in docker and kubernetes is also the same.

loveyana · 2021-12-03T11:13:32Z

There is also an immature idea.I mentioned such a problem before, #2573, spiffe-csi uses csi-driver to build grpc communication from other services to spire-agent, and some paths to mount it.
I think we can insert certain specified fields into the "xattr" of the file in the disk plug-in, maybe also in the entry definition, so that we can make the pod only The purpose of being able to mount to its own svid file and socket, do you have any suggestions for this.

Looking forward for your response, thank you~

Signed-off-by: Agustín Martínez Fayó <[email protected]>

… attribute namespaces" section in https://man7.org/linux/man-pages/man7/xattr.7.html Signed-off-by: Agustín Martínez Fayó <[email protected]>

amartinezfayo · 2021-12-03T18:47:52Z

Thank you @loveyana for looking at this PR!

the constant spire-svid is used as the attribute name in your plug-in, do you mind modifying this problem, regard it as a configuration or modify it as a user namespace attribute

Thanks for pointing that the namespace was missing. It now uses the user namespace when setting the extended attribute.

I think we can insert certain specified fields into the "xattr" of the file in the disk plug-in, maybe also in the entry definition, so that we can make the pod only The purpose of being able to mount to its own svid file and socket, do you have any suggestions for this.

If I'm understanding correctly, you are suggesting to encode in selectors information that will end up being set as extended attributes in the (SVID) files that the plugin writes to disk. If that's the case, I would be hesitant to provide that kind of functionality that essentially would allow to set arbitrary data in the extended attributes that would be consumed for a certain purpose.
Generally speaking, I think that the plugin should serve the specific purpose of storing SVIDs on disk, and handling that kind of functionality feels like out of the scope of the plugin and can lead to compatibility problems and make it more error prone.

amoore877 · 2021-12-03T20:44:46Z

pkg/agent/plugin/svidstore/disk/disk.go

+	attrName   = "user.spire-svid"
+)
+
+type certFile struct {


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

amoore877 · 2021-12-03T20:48:21Z

pkg/agent/plugin/svidstore/disk/disk.go

+	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)
+	}


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

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?

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.

loveyana · 2021-12-05T16:43:05Z

@amartinezfayo I am glad to receive your reply, and thank you that you have solved this problem, I have passed the test in the cluster.

Regarding the second half of the reply, I actually still have some thoughts I want to share with you. In our current environment, if we use the svidstore plug-in for svid storage, we definitely hope that each service can only get its own svid.

I think that in the gcp or aws svidstore scenario, we have solved this problem in disguise through the cloud platform's permission management, but in the disk svidstore scenario, in the current design, I completely believe that there is no way to solve this problem.

So I have a suggestion, whether we can specify a way like -selector "disk:xattr:serviceaccount:example-service" when the entry is created, by inserting user.<plugin-prefix>-serviceaccount to example service svids files, It is left to the platforms that distribute these svids, including kubernetes or any virtualization platform to determine whether to issue certificates. I think this is also the original intention of spiffe in designing the selector mechanism.

MarcosDY · 2021-12-06T16:28:04Z

pkg/agent/plugin/svidstore/disk/disk.go

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


lcoks are not required because it is done in plugin level

MarcosDY · 2021-12-06T16:31:03Z

pkg/agent/plugin/svidstore/disk/disk.go

+
+	p.trustDomain = req.CoreConfiguration.TrustDomain
+	p.config = config
+	if config.Directory == "" {


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

MarcosDY · 2021-12-06T16:36:24Z

pkg/agent/plugin/svidstore/disk/disk.go

+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)


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.

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.

MarcosDY · 2021-12-06T18:25:38Z

pkg/agent/plugin/svidstore/disk/disk.go

+	return p.config, nil
+}
+
+func (c *certFile) write(attrValue string) error {


functions must be moved together with struct definition

MarcosDY · 2021-12-06T19:17:28Z

pkg/agent/plugin/svidstore/disk/disk.go

+// 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) {


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

MarcosDY · 2021-12-06T21:34:23Z

pkg/agent/plugin/svidstore/disk/xattr_fallback.go

@@ -0,0 +1,15 @@
+// +build !linux
+// +build !darwin


what do you think about 1 file for windows and another for !windows?
we are using that approach in another places, and may works

The problem is that there are non-Windows platforms where this is not supported, so we can't just have !windows for the supported implementation. The supported ones are: linux, darwin, freebsd and netbsd. I see that I missed freebsd and netbsd., which I should add.

MarcosDY · 2021-12-06T21:36:41Z

pkg/agent/plugin/svidstore/disk/xattr_fallback.go

+
+func setxattr(filePath, attr string, data []byte) error {
+	// On unsupported systems, we just do nothing
+	return nil


you forgot to close {

MarcosDY · 2021-12-06T21:36:43Z

pkg/agent/plugin/svidstore/disk/xattr_fallback.go

+	// On unsupported systems, we just do nothing
+	return nil
+
+func getxattr(filePath, attr string, dest []byte) (err error) {


maybe:

func getxattr(filePath, attr string, dest []byte) error {

MarcosDY · 2021-12-06T21:37:22Z

pkg/agent/plugin/svidstore/disk/xattr_unix.go

+	return nil
+}
+
+func getxattr(filePath, attr string, dest []byte) (err error) {


same as before you are not using err var

MarcosDY · 2021-12-06T21:38:43Z

pkg/agent/svid/store/service.go

-	log = log.WithField(telemetry.SVIDStore, storeName)
+	log = log.WithFields(logrus.Fields{
+		telemetry.SVIDStore: storeName,
+		telemetry.Entry:     entry.EntryId,


why are you replacing log here? it is defined above and contains EntryID and SPIFFEID

Thanks for catching this. I was somehow confused about missing the entry ID and SPIFFE ID.

azdagron · 2021-12-06T23:45:35Z

I wonder if there is an alternative here than the use of extended attributes. Support for xattrs is very spotty based on the filesystem and/or platform. Considering the wide scope of SPIRE deployments, I think we'd be better off if we can figure something else out rather than have a divergent experience.

Signed-off-by: Agustín Martínez Fayó <[email protected]>

- Set proper ownership and permissions - Do not use extended attributes Signed-off-by: Agustín Martínez Fayó <[email protected]>

Signed-off-by: Agustín Martínez Fayó <[email protected]>

MarcosDY · 2022-01-06T14:48:39Z

doc/plugin_agent_svidstore_disk.md

-| `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. |
+| `disk:certchainfile`      | `disk:certchainfile:tls.crt`   | x        | The file path relative to the base directory where the SVID certificate chain will be stored. Must be in the same directory as `keyfile` and `bundlefile`. |


I'm not sure about setting path in each selector, we are forced to set all selectors with paths and make sure all paths are the same.
What do you think about creating a selector to set a folder in a single place?
that will allow us to have "default" file names, so entries will be easier, and we can have other selectors to set file names.
example using default names:

{ "spiffeID": "spiffe://example.org/w1", "parentID": "spiffe://example.org/agent", "selectors", [ "disk:directory:/tmp" ] }

example changing a single file name

{ "spiffeID": "spiffe://example.org/w1", "parentID": "spiffe://example.org/agent", "selectors", [ "disk:certchainfile:tls.crt" ] }

I thought about this and I didn't like the idea of adding a new selector.
Now that I think it more, this also has the benefit of being able to have default file names and end up with a less number of required selectors. I'll make the change to implement it in the suggested way.

MarcosDY · 2022-01-06T14:55:38Z

doc/plugin_agent_svidstore_disk.md

+| `disk:certchainfile`      | `disk:certchainfile:tls.crt`   | x        | The file path relative to the base directory where the SVID certificate chain will be stored. Must be in the same directory as `keyfile` and `bundlefile`. |
+| `disk:keyfile` | `disk:keyfile:key.crt` | x        | The file path relative to the base directory where the SVID certificate key will be stored. Must be in the same directory as `certchainfile` and `bundlefile`. |
+| `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. Must be in the same directory as `certchainfile` and `keyfile`. |
+| `disk:group`     | `disk:group:my-workload` | x (if `gid` is not specified)       | The group name that is set to the files written to disk. If set, `gid` cannot be specified. |


disk:group and disk:gid is confusing, may we use groupname or gname instead? and have a consistent name for gid?

Sounds good. I like groupname and groupid.

MarcosDY · 2022-01-06T15:22:47Z

pkg/agent/plugin/svidstore/disk/disk.go

-	if err := createBaseDirectoryIfNeeded(c.filePath); err != nil {
+	// If needed, create the parent directory that will contain the
+	// autogenerated directory and the symbolic link.
+	parentDir := filepath.Dir(originalDir)


this means that agent user must have writing rights to parent folder,
if we have a selector disk:certchain:/usr/certs/cert.pem user must have write rights to /usr and then we try to change parent permissions, no matter if we created the folder or it exists.
I think we must never change folder permission, and only change it when we create that folder.

Thanks for catching this! Changing permissions on the parent directory (out of the plugin directory space) was not the intention. I'll fix that.

MarcosDY · 2022-01-06T15:31:48Z

pkg/agent/plugin/svidstore/disk/disk.go

+	// directory is not completed. If there is an error before
+	// finishing writing all the files, the directory must be
+	// cleaned up.
+	d.cleanupDir = autoGeneratedDir


why an attribute to struct level is required here? is it possible to have another value? and we remove even if function fail before?

Yes, it's better to handle this with a var in the function. I'll make that change.

MarcosDY · 2022-01-06T15:38:38Z

pkg/agent/plugin/svidstore/disk/disk.go

+
+	keyFile := filepath.Join(autoGeneratedDir, filepath.Base(d.key.filePath))
+	log.With("key_file_path", keyFile).Debug("Writing key file")
+	if err := os.WriteFile(keyFile, d.key.pemBytes, permFile); err != nil {


can you add a comment about why keys uses 640 permissions? I understand it is done this way because workload must have access to key.

MarcosDY · 2022-01-06T15:51:54Z

pkg/agent/plugin/svidstore/disk/disk.go

+	// Defer a function that cleanups the files that are no
+	// longer used.
+	defer func() {
+		if removeAllErr := os.RemoveAll(d.cleanupDir); removeAllErr != nil {


nit: validate d.cleanupDir != ""

calling os.RemoveAll with an empty string causes no error.

MarcosDY · 2022-01-06T15:54:09Z

pkg/agent/plugin/svidstore/disk/disk.go

+		return status.Errorf(codes.Internal, "failed to rename symbolic link: %v", err)
+	}
+
+	if oldPath != "" {


you can just assign oldPath, since you are always using oldPath value, and maybe add a comment about what happens here

MarcosDY · 2022-01-06T16:14:22Z

pkg/agent/plugin/svidstore/disk/disk.go

+	gid := metadataMap["gid"]
+	groupName := metadataMap["group"]
+
+	if gid != "" {


I think it can be simplified with a switch:

swtich { case gid != "" && groupName != "": return nil, status.Error(codes.InvalidArgument, "either gid or group must be specified, not both") case gid != "": .... case groupName != "" : ..... }

Signed-off-by: Agustín Martínez Fayó <[email protected]>

amartinezfayo · 2022-01-12T00:21:08Z

This PR evidences that the configuration and use of this plugin as it has been implemented here can be cumbersome and most likely will result in operational problems. More details can be found here: #2077 (comment)
Closing it as a result of that.

amartinezfayo added 3 commits December 2, 2021 21:33

SVIDStore disk plugin

eb457e7

Signed-off-by: Agustín Martínez Fayó <[email protected]>

SVIDStore disk plugin

adabc13

Signed-off-by: Agustín Martínez Fayó <[email protected]>

SVIDStore disk plugin

4a450b4

Signed-off-by: Agustín Martínez Fayó <[email protected]>

amartinezfayo requested review from APTy, azdagron, evan2645 and rturner3 as code owners December 3, 2021 00:55

amartinezfayo added 3 commits December 3, 2021 08:48

SVIDStore disk plugin

a3e4091

Signed-off-by: Agustín Martínez Fayó <[email protected]>

SVIDStore disk plugin

1ed9fce

Signed-off-by: Agustín Martínez Fayó <[email protected]>

Use the user namespace in the extended attribute. See the "Extended…

f85e6cf

… attribute namespaces" section in https://man7.org/linux/man-pages/man7/xattr.7.html Signed-off-by: Agustín Martínez Fayó <[email protected]>

amoore877 reviewed Dec 3, 2021

View reviewed changes

MarcosDY reviewed Dec 6, 2021

View reviewed changes

Address PR comments

81c022d

Signed-off-by: Agustín Martínez Fayó <[email protected]>

azdagron added this to the 1.2.0 milestone Dec 7, 2021

amartinezfayo added 4 commits January 5, 2022 15:23

- Provide atomicity to the write operation

b159137

- Set proper ownership and permissions - Do not use extended attributes Signed-off-by: Agustín Martínez Fayó <[email protected]>

Merge remote-tracking branch 'origin/main' into svidstore-disk

bbfab82

Update tests

9c2c400

Signed-off-by: Agustín Martínez Fayó <[email protected]>

Update documentation

331f5dd

Signed-off-by: Agustín Martínez Fayó <[email protected]>

MarcosDY reviewed Jan 6, 2022

View reviewed changes

Address PR comments

22f659b

Signed-off-by: Agustín Martínez Fayó <[email protected]>

amartinezfayo mentioned this pull request Jan 12, 2022

Serverless support: disk plugin #2077

Closed

amartinezfayo closed this Jan 12, 2022

SVIDStore disk plugin #2647

SVIDStore disk plugin #2647

Conversation

amartinezfayo commented Dec 3, 2021

loveyana commented Dec 3, 2021 • edited Loading

loveyana commented Dec 3, 2021

amartinezfayo commented Dec 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loveyana commented Dec 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azdagron commented Dec 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amartinezfayo commented Jan 12, 2022

SVIDStore `disk` plugin #2647

SVIDStore `disk` plugin #2647

loveyana commented Dec 3, 2021 •

edited

Loading

loveyana commented Dec 5, 2021 •

edited

Loading