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

zveinn · 2024-03-21T15:47:55Z

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 · 2024-03-21T15:57:51Z

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

zveinn · 2024-03-21T16:03:21Z

@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 · 2024-03-21T16:38:07Z

🤔 @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 · 2024-03-21T17:47:38Z

🤔 @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 · 2024-03-21T17:50:30Z

🤔 @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 minio/operator#2032 (comment)

zveinn · 2024-03-21T17:50:49Z

🤔 @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.

shtripat

initial minor ones

cmd/cp-main.go

cmd/encryption-methods.go

cmd/get-main.go

cmd/mirror-main.go

cmd/mv-main.go

cmd/put-main.go

cmd/rm-main.go

cmd/typed-errors.go

klauspost

Just some minor stuff.

cmd/encryption-methods.go

cmd/error.go

cmd/head-main.go

cmd/pipe-main.go

docs/MAINTAINERS.md

klauspost

LGTM

cmd/encryption-methods.go

cmd/encryption-methods_test.go

cmd/error.go

cmd/flags.go

cmd/get-main.go

cmd/pipe-main.go

go.mod

zveinn · 2024-04-11T12:27:53Z

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.

shtripat

few minor ones only

cmd/cat-main.go

cmd/cp-main.go

cmd/flags.go

cmd/head-main.go

cmd/mirror-main.go

Change encrypt-key to enc-c Keys cannot end in "=" See minio/mc#4882

zveinn requested review from vadmeste, harshavardhana, klauspost, aead and jiuker March 21, 2024 15:47

zveinn mentioned this pull request Mar 21, 2024

[mc] This PR will lead to big documentation updates on the website minio/docs#1165

Closed

zveinn force-pushed the sse-adjustments branch from 33bbcae to 5886d51 Compare March 21, 2024 16:07

zveinn force-pushed the sse-adjustments branch from 2242209 to 9a36d74 Compare March 21, 2024 17:32

shtripat reviewed Mar 22, 2024

View reviewed changes

bh4t assigned zveinn Mar 25, 2024

bh4t added the new feature label Mar 25, 2024

klauspost reviewed Apr 9, 2024

View reviewed changes

zveinn force-pushed the sse-adjustments branch 2 times, most recently from a33beaf to d7ea482 Compare April 10, 2024 12:50

zveinn changed the title ~~[ DRAFT ] Large MC Update~~ Large MC Update ( encryption flags, functional test suite, removal of session code, minor cleanup, vuln. updates ) Apr 10, 2024

klauspost approved these changes Apr 10, 2024

View reviewed changes

jiuker approved these changes Apr 10, 2024

View reviewed changes

harshavardhana requested changes Apr 11, 2024

View reviewed changes

zveinn force-pushed the sse-adjustments branch from 12b785e to 44a359f Compare April 11, 2024 11:15

zveinn requested review from harshavardhana, shtripat and klauspost April 11, 2024 12:24

zveinn requested a review from jiuker April 11, 2024 12:29

shtripat reviewed Apr 11, 2024

View reviewed changes

cmd/cat-main.go Show resolved Hide resolved

cmd/cp-main.go Outdated Show resolved Hide resolved

cmd/flags.go Show resolved Hide resolved

cmd/head-main.go Outdated Show resolved Hide resolved

cmd/mirror-main.go Outdated Show resolved Hide resolved

klauspost approved these changes Apr 11, 2024

View reviewed changes

shtripat approved these changes Apr 11, 2024

View reviewed changes

jiuker approved these changes Apr 12, 2024

View reviewed changes

zveinn force-pushed the sse-adjustments branch from addf145 to 081bc27 Compare April 15, 2024 12:01

Update version for govulncheck (minio#4898) and master rebase

5dd72b7

zveinn force-pushed the sse-adjustments branch from 081bc27 to 5dd72b7 Compare April 15, 2024 12:02

mod update

0cc2c50

harshavardhana approved these changes Apr 15, 2024

View reviewed changes

harshavardhana merged commit fe58afc into minio:master Apr 15, 2024
7 checks passed

zveinn mentioned this pull request Apr 15, 2024

updating tests to use new mc --enc flags minio/minio#19508

Merged

harshavardhana mentioned this pull request Apr 19, 2024

mc --enc-{c,s3,kms) require that input to match the argument #4906

Closed

allanrogerr added a commit to minio/docs that referenced this pull request Oct 22, 2024

Update server-side-encryption-sse-c.rst

705743a

Change encrypt-key to enc-c Keys cannot end in "=" See minio/mc#4882

allanrogerr mentioned this pull request Oct 22, 2024

Update server-side-encryption-sse-c.rst minio/docs#1355

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zveinn commented Mar 21, 2024 •

edited

Loading

ravindk89 commented Mar 21, 2024

zveinn commented Mar 21, 2024 •

edited

Loading

ravindk89 commented Mar 21, 2024

feorlen commented Mar 21, 2024

ravindk89 commented Mar 21, 2024

zveinn commented Mar 21, 2024

shtripat left a comment

klauspost left a comment

klauspost left a comment

zveinn commented Apr 11, 2024

shtripat left a comment

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

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

Conversation

zveinn commented Mar 21, 2024 • edited Loading

Community Contribution License

Note

The test suite

MC updates

Things that were fixed/changed

Changed encryption flag functionality

Deprecated flags

New flags

Types of changes

Checklist:

ravindk89 commented Mar 21, 2024

zveinn commented Mar 21, 2024 • edited Loading

ravindk89 commented Mar 21, 2024

feorlen commented Mar 21, 2024

ravindk89 commented Mar 21, 2024

zveinn commented Mar 21, 2024

shtripat left a comment

Choose a reason for hiding this comment

klauspost left a comment

Choose a reason for hiding this comment

klauspost left a comment

Choose a reason for hiding this comment

zveinn commented Apr 11, 2024

shtripat left a comment

Choose a reason for hiding this comment

zveinn commented Mar 21, 2024 •

edited

Loading

zveinn commented Mar 21, 2024 •

edited

Loading