Change CreatedAt types for KMS from string to time.Time

[marktheunissen]

Discussion here: minio/minio#20223 (comment)

Other pull requests:

• Change KMSKeyInfo.CreatedAt to a time.Time console#3421

[klauspost]

Are any of the fields currently used? A bit worried about old minio + new mc.

[ramondeklein]

[ramondeklein]

@klauspost You're right. The most common used client would be mc and because the JSON marshalling wasn't set to omitempty, it will probably contain an empty string when used with old MinIO clients. We may want to use custom unmarshalling that is able to deal with an empty string and translate it to "no time".

[ramondeklein]

Please add custom unmarshalling for:

func (k *KMSKeyInfo) UnmarshalJSON(data []byte) error
func (k *KMSPolicyInfo) UnmarshalJSON(data []byte) error
func (k *KMSIdentityInfo) UnmarshalJSON(data []byte) error
func (k *KMSDescribePolicy) UnmarshalJSON(data []byte) error
func (k *KMSDescribeIdentity) UnmarshalJSON(data []byte) error

[klauspost]

https://go.dev/play/p/7RUcboegpkz - yeah... It even triggers an error on empty strings :(

[marktheunissen]

Are any of the fields currently used?

4/5 of the structs aren't used anywhere meaningfully, that I can see. Think they are just vestigal. I would prefer to remove them, if we can? Please check my searches below:

• KMSDescribeIdentity: https://github.com/search?q=%28org%3Aminio+OR+org%3Aminiohq%29+KMSDescribeIdentity&type=code&ref=advsearch
• KMSDescribePolicy: https://github.com/search?q=%28org%3Aminio+OR+org%3Aminiohq%29+KMSDescribePolicy&type=code&ref=advsearch
• KMSIdentityInfo: https://github.com/search?q=%28org%3Aminio+OR+org%3Aminiohq%29+KMSIdentityInfo&type=code&ref=advsearch
• KMSPolicyInfo: https://github.com/search?q=%28org%3Aminio+OR+org%3Aminiohq%29+KMSPolicyInfo&type=code&ref=advsearch

The only place that catches my attention is in miniohq/minio:master which is a 2 year old master branch fork. I think that maybe the functionality once existed in MinIO and was removed.

The most common used client would be mc and because the JSON marshalling wasn't set to omitempty, it will probably contain an empty string when used with old MinIO clients.

For KMSKeyInfo, by sheer luck and for an unknown reason, the current JSON marshalling is done with a different struct kes.KeyInfo, which uses a time.Time for CreatedAt, see: https://github.com/minio/minio/blob/master/cmd/kms-handlers.go#L224-L234

Which means existing builds of MinIO respond as follows:

127.0.0.1:9000 GET /minio/kms/v1/key/list?pattern=%2A
127.0.0.1:9000 Proto: HTTP/1.1
127.0.0.1:9000 Host: 127.0.0.1:9000
...
127.0.0.1:9000 [RESPONSE] [2024-08-09T10:55:45.447] [ Duration 637µs TTFB 636µs ↑ 93 B  ↓ 60 B ]
127.0.0.1:9000 200 OK
127.0.0.1:9000 Content-Type: application/json
127.0.0.1:9000 Accept-Ranges: bytes
127.0.0.1:9000 Content-Length: 60
127.0.0.1:9000 Server: MinIO
127.0.0.1:9000 [{"name":"default-key","created_at":"0001-01-01T00:00:00Z"}]
127.0.0.1:9000

(I would paste some test commands for play.min.io here but it's currently offline for maintenance)

The struct kes.KeyInfo does use omitempty in Marshalling, however it is not effective on time.Times, which have non-empty zero value: https://github.com/minio/kms-go/blob/main/kes/key.go#L88-L110

To be effective it would have had to use time.IsZero and a pointer, here is how it should have worked: (comment/uncomment the Marshal function: https://play.golang.com/p/zCfC8oopWiu). Do we want to reproduce this pattern on KMSKeyInfo? It would probably be cleaner to other languages to omit the created_at key, but for Go, I'm not sure how much it really matters since Go can detect zero value timestamps.

So what about other KMSKeyInfo usage? Here is a search:

• KMSKeyInfo: https://github.com/search?q=%28org%3Aminio+OR+org%3Aminiohq%29+KMSKeyInfo&type=code&ref=advsearch

I don't see anything noteworthy in the above results.

This is a bit of a rabbit hole, I think to summarize my preferred actions are just to remove all the structs except KMSKeyInfo and not do any override of the unmarshalling.

If you guys spot errors or potential issues in my reasoning, please link directly to lines of code so I can follow up and test, thanks.

[ramondeklein]

Please check my searches below:

• https://github.com/search?q=%28org%3Aminio+OR+org%3Aminiohq%29+KMSDescribeIdentity&type=code&ref=advsearch
• KMSDescribePolicy: https://github.com/search?q=%28org%3Aminio+OR+org%3Aminiohq%29+KMSDescribePolicy&type=code&ref=advsearch
• KMSIdentityInfo: https://github.com/search?q=%28org%3Aminio+OR+org%3Aminiohq%29+KMSIdentityInfo&type=code&ref=advsearch
• KMSPolicyInfo: https://github.com/search?q=%28org%3Aminio+OR+org%3Aminiohq%29+KMSPolicyInfo&type=code&ref=advsearch

Note that these searches are not really valid. If you call madmin.ListKeys(), then it returns []madmin.KMSKeyInfo, but it won't show up in this search. But I can confirm that CreatedBy is never accessed from inside mc.

I don't think we will have any issues. Go serializes a time.Time structure as a string too ("2009-11-10T23:00:00Z"). So even if clients expect a string, then they'll get a string.

[marktheunissen]

@klauspost Happy with the above? :)