Large MC Update ( encryption flags, functional test suite, removal of session code, minor cleanup, vuln. updates )

[zveinn]

Community Contribution License

All community contributions in this pull request are licensed to the project maintainers under the terms of the Apache 2 license. By creating this pull request I represent that I have the right to license the contributions to the project maintainers under the Apache 2 license.

Note

This is a rather large update to the mc so let's take our time to review and discuss

The test suite

The new test suite is created to run an mc binary against an endpoint. The idea is to be able to run any version of mc against any version of minio by just changing .env variables. Our ci pipeline is currently not set up for that, so the test suite will not run automatically for now.

The test suite is a replacement for the bash functional suite we had before and was hard to modify and maintain. The new suite covers all tests cases that the bash test suite used to cover, hence why it took so long to submit this PR. Along with the old test cases we have a lot of new ones as well, especially regarding the new encryption flags.

MC updates

This PR contains a few items that needed a fix and have been sittin in the backlog. Normally I would not bundle so many things together but since the core change for this PR was to deprecate the --encrypt --encrypt-kms and --encrypt-s3 flags it was a good chance to also include the new functional test suite to cover the changes.

During the update I discovered that some of the mc methods have documentation stating that they can accept encryption flags, but in reality these methods do not need encryption at all. This lead to a small refactor of certain methods.

Another change that kind of just happened during this refactor was the removal of the --continue flag. This flag was behaving in an insecure way and after a discussion with AB, we decided to completely remove this code from the mc client.

Things that were fixed/changed

• Removes deprecated documentation and updated Readme.md
• '--continue' flag has been deprecated
• '--encrypt' flag is now called '--enc-c'
• '--encrypt-s3' flag is now called '--enc-s3'
• '--encrypt-kms' flag is now called '--enc-kms'
• Previous encrypt flag parsing was broken and allowed for invalid keys to be accepted as valid ones.
• Help menu documentaion on encryption flags (and some others) has been updated
• Removed encryption flags/code from methods that do not need encryption to function

Changed encryption flag functionality

The new encryption flags (--enc-s3, --enc-kms, --enc-c) will only accept RawBase64 encoded keys and the cli parameters are now '[]string' instead of 'string'. This allows us to properly parse the keys.

Deprecated flags

• '--encrypt'
• '--encrypt-key'
• '--continue'

New flags

• '--enc-c'
• '--enc-kms'
• '--enc-s3'

Types of changes

• [ x ] Bug fix (non-breaking change which fixes an issue)
• [ x ] New feature (non-breaking change which adds functionality)
• [ x ] Optimization (provides speedup with no functional changes)
• [ x ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• [ x ] Unit tests added/updated
• [ x ] Internal documentation updated
• [ x ] Create a documentation update request here

[ravindk89]

@zveinn is it a hard requirement for us to rename --encrypt<method> to --enc-<method> ?

[zveinn]

@zveinn is it a hard requirement for us to rename --encrypt<method> to --enc-<method> ?

It's not a hard requirement, but even if we rename the flags they will not function the same. So it's better to change the name entirely to prevent confusion. Originally we were going to keep backwards compatibility on this, but we decided that was not a good idea.

But I am neutral on the renaming tbh, if we want to keep the old names then that's all good.

[ravindk89]

:thinking: @djwfyi @feorlen I'm not of a strong opinion - it's more important that we keep consistent moving forward.. We can document it either way.

[feorlen]

🤔 @djwfyi @feorlen I'm not of a strong opinion - it's more important that we keep consistent moving forward.. We can document it either way.

Do we have a schedule to deprecate then remove for these? I'm fine with name changes as long as we are able to handle the transition in an orderly fashion.

[ravindk89]

🤔 @djwfyi @feorlen I'm not of a strong opinion - it's more important that we keep consistent moving forward.. We can document it either way.

Do we have a schedule to deprecate then remove for these? I'm fine with name changes as long as we are able to handle the transition in an orderly fashion.

@kannappanr it might be nice to flesh out how exactly we want to time deprecations, this conversation is also at hand in https://github.com/minio/operator/pull/2032#issuecomment-2010034521

[zveinn]

🤔 @djwfyi @feorlen I'm not of a strong opinion - it's more important that we keep consistent moving forward.. We can document it either way.

Do we have a schedule to deprecate then remove for these? I'm fine with name changes as long as we are able to handle the transition in an orderly fashion.

I don't think we're doing a deprecation phase for this one.

[zveinn]

Just did a small refactor for the encryption key parsing, there was a method that was forcing the use of only one type of key at a time. It's been removed and the code paths simplified.