FileStorage Delete doesn't delete non-empty directories

[goksan]

What version of the package are you using?

v0.20.0

What are you trying to do?

I'm trying to use the Delete method on FileStorage to delete all certificates for a given issuer.

What steps did you take?

var testCache *certmagic.Cache
testCache = certmagic.NewCache(certmagic.CacheOptions{
	GetConfigForCert: func(cert certmagic.Certificate) (*certmagic.Config, error) {
		return certmagic.New(testCache, certmagic.Config{}), nil
	},
})

testMagic := certmagic.New(testCache, certmagic.Config{})

testACME := certmagic.NewACMEIssuer(testMagic, certmagic.ACMEIssuer{
	CA:     certmagic.LetsEncryptStagingCA,
	Email:  " ",
	Agreed: true,
})

err := testMagic.Storage.Delete(context.Background(), path.Join("certificates", testACME.IssuerKey()))
if err != nil {
	log.Printf("testMagic.Storage.Delete: %s", err)
}

What did you expect to happen, and what actually happened instead?

I expected successful deletion of the directory based on these comments:

certmagic/storage.go

Lines 82 to 87 in c61a4fe

	// Delete deletes the named key. If the name is a
	// directory (i.e. prefix of other keys), all keys
	// prefixed by this key should be deleted. An error
	// should be returned only if the key still exists
	// when the method returns.
	Delete(ctx context.Context, key string) error

Instead I saw:

testMagic.Storage.Delete: remove certmagic/certificates/acme-staging-v02.api.letsencrypt.org-directory: directory not empty

How do you think this should be fixed?

We could change Delete in filestorage.go to call os.RemoveAll rather than os.Remove

Before:

// Delete deletes the value at key.
func (s *FileStorage) Delete(_ context.Context, key string) error {
	return os.Remove(s.Filename(key))
}

After:

// Delete deletes the value at key.
func (s *FileStorage) Delete(_ context.Context, key string) error {
	return os.RemoveAll(s.Filename(key))
}

It's also possible that the current behaviour is indeed desired, and the comments are outdated or misunderstood by myself.

[mholt]

That is a feature, IIRC. If you look at the maintenance routines we only delete dirs after first deleting their contents deliberately. The idea is to prevent accidentally deleting useful files.

It's also simpler to implement in general (databases and such).

[goksan]

I think I get you, I guess where I went wrong was with misunderstanding the following:

"If the name is a directory (i.e. prefix of other keys), all keys prefixed by this key should be deleted."

certmagic/storage.go

Lines 82 to 87 in c61a4fe

	// Delete deletes the named key. If the name is a
	// directory (i.e. prefix of other keys), all keys
	// prefixed by this key should be deleted. An error
	// should be returned only if the key still exists
	// when the method returns.
	Delete(ctx context.Context, key string) error

I understood that to mean that implementations would delete all keys prefixed by a key, but it sounds like that's intended to convey that all keys prefixed by this key should already be deleted.

What do you think of something like this?

// Delete deletes the named key. If the name is a 
// directory (i.e. a prefix for other keys), the named 
// key must not be in use as a prefix in any existing 
// keys. An error should be returned only if the key still 
// exists when the method returns.

Curious to know whether you can see how I got there, or whether you think it should already be clear.

[mholt]

That's a good point... hmm. As I think about it, I think the godoc should be correct, and maybe just fixing the file system implementation is best. Because otherwise every implementation needs to check if the "directory" is "empty" (query by prefix, etc) -- whereas we're just lucky the file system does this for us. But databases, etc, don't do that.

So yeah, we should probably change it to use RemoveAll(). It might be good if we have some sort of sanity check on the resulting path.

[goksan]

Was there a particular sanity check you had in mind?

Maybe you were thinking about checking the directory exists? This currently should error with Remove but wouldn't after swapping it for RemoveAll, is that the concern?

[mholt]

Hmm, not exactly... I am not sure I had a concrete idea for a sanity check. In theory all the paths used should be safe. 🤷‍♂️

[goksan]

I've done the easy work of opening a PR if you think this change would be beneficial, thanks for discussing.

[mholt]

Yeah, thank you!

I might see what other implementations are doing in this regard, and possibly make adjustments if needed, but this is good for now. Thanks!