New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stage0 break apart --insecure-skip-verify #1738

Merged
merged 9 commits into from Nov 27, 2015

Conversation

Projects
None yet
4 participants
@blixtra
Collaborator

blixtra commented Nov 13, 2015

This PR breaks up the --insecure-skip-verify flag. The new flag is called --insecure-options and accepts these flags: "none" (default), "image", "tls", "fingerprint", and "all" (same as --insecure-skip-verify).

Along the way new type were created to work with csv params and some refactoring happened.

I'm putting this up know that there is still a bit of work todo regarding documentation (See below).

TODO: Document the flags and Make tests and documentation use the more granular options instead of 'all'

fixes #912

@blixtra

This comment has been minimized.

Show comment
Hide comment
@blixtra

blixtra Nov 13, 2015

Collaborator

Ignore this for now. Something went wrong with my last minute rebase.

Collaborator

blixtra commented Nov 13, 2015

Ignore this for now. Something went wrong with my last minute rebase.

@blixtra

This comment has been minimized.

Show comment
Hide comment
@blixtra

blixtra Nov 16, 2015

Collaborator

Would welcome initial reviews now.

Collaborator

blixtra commented Nov 16, 2015

Would welcome initial reviews now.

Show outdated Hide outdated Documentation/networking.md Outdated
@iaguis

This comment has been minimized.

Show comment
Hide comment
@iaguis

iaguis Nov 17, 2015

Member

About the "lastusedtime -> lastused", you can't directly change the name of the field in the database without doing a migration function and all that stuff.

I suggest to make everything lastusedtime except the user-facing parts.

Member

iaguis commented Nov 17, 2015

About the "lastusedtime -> lastused", you can't directly change the name of the field in the database without doing a migration function and all that stuff.

I suggest to make everything lastusedtime except the user-facing parts.

@blixtra

This comment has been minimized.

Show comment
Hide comment
@blixtra

blixtra Nov 17, 2015

Collaborator

@iaguis I'd rather write the migration function than allow legacy stuff to start piling up pre-1.0. I understand the only disadvantage is when moving from to an older version of rkt.

Collaborator

blixtra commented Nov 17, 2015

@iaguis I'd rather write the migration function than allow legacy stuff to start piling up pre-1.0. I understand the only disadvantage is when moving from to an older version of rkt.

Show outdated Hide outdated rkt/cli_apps.go Outdated
Show outdated Hide outdated rkt/cli_apps.go Outdated
Show outdated Hide outdated rkt/cli_apps.go Outdated
Show outdated Hide outdated rkt/cli_apps_test.go Outdated
Show outdated Hide outdated rkt/cli_apps.go Outdated
Show outdated Hide outdated rkt/image_list.go Outdated
Show outdated Hide outdated rkt/cli_apps.go Outdated
Show outdated Hide outdated rkt/fetch_test.go Outdated
Show outdated Hide outdated rkt/rkt.go Outdated
@iaguis

This comment has been minimized.

Show comment
Hide comment
@iaguis

iaguis Nov 20, 2015

Member

Since you're using specific options in the tests and docs, you can update the commit titles.

Member

iaguis commented Nov 20, 2015

Since you're using specific options in the tests and docs, you can update the commit titles.

@iaguis

This comment has been minimized.

Show comment
Hide comment
@iaguis

iaguis Nov 20, 2015

Member

In rkt trust we're checking for the insecure-fingerprint flag to refuse executing it if it's passed. However, further down we're using that flag to force accepting the key, this never gets executed because we bail out before. I'm not entirely sure how we got to this situation but the check was added ito fix #1285.

I think SkipFingerprintCheck should be a rkt trust option only that disables prompting the user for a confirmation before trusting a key.

I'm not sure if it should stay global as a part of --insecure-options or we should make it rkt trust-specific in the UI too.

Member

iaguis commented Nov 20, 2015

In rkt trust we're checking for the insecure-fingerprint flag to refuse executing it if it's passed. However, further down we're using that flag to force accepting the key, this never gets executed because we bail out before. I'm not entirely sure how we got to this situation but the check was added ito fix #1285.

I think SkipFingerprintCheck should be a rkt trust option only that disables prompting the user for a confirmation before trusting a key.

I'm not sure if it should stay global as a part of --insecure-options or we should make it rkt trust-specific in the UI too.

Show outdated Hide outdated rkt/rkt.go Outdated
Show outdated Hide outdated rkt/images.go Outdated
Show outdated Hide outdated Documentation/commands.md Outdated
@alban

This comment has been minimized.

Show comment
Hide comment
@alban

alban Nov 25, 2015

Member

Now that #1793 is merged, please update CHANGELOG.md in this PR. Since --insecure-skip-verify will not work anymore, we should explain how to migrate scripts.

Member

alban commented Nov 25, 2015

Now that #1793 is merged, please update CHANGELOG.md in this PR. Since --insecure-skip-verify will not work anymore, we should explain how to migrate scripts.

Show outdated Hide outdated Documentation/commands.md Outdated
@alban

This comment has been minimized.

Show comment
Hide comment
@alban

alban Nov 26, 2015

Member

When reviewing on GitHub, GitHub just says "No changes" for most of the files, e.g. rkt/images.go. But it is obviously not true when I look at the branch directly from git. Any idea why this breaks GitHub?

Member

alban commented Nov 26, 2015

When reviewing on GitHub, GitHub just says "No changes" for most of the files, e.g. rkt/images.go. But it is obviously not true when I look at the branch directly from git. Any idea why this breaks GitHub?

@krnowak

This comment has been minimized.

Show comment
Hide comment
@krnowak

krnowak Nov 26, 2015

Collaborator

Probably too many changes for poor github to cope. Not sure how can I review it now.

Collaborator

krnowak commented Nov 26, 2015

Probably too many changes for poor github to cope. Not sure how can I review it now.

@iaguis

This comment has been minimized.

Show comment
Hide comment
@iaguis

iaguis Nov 26, 2015

Member

Maybe commenting in the commits Hehe, too late.

Member

iaguis commented Nov 26, 2015

Maybe commenting in the commits Hehe, too late.

@krnowak

This comment has been minimized.

Show comment
Hide comment
@krnowak

krnowak Nov 27, 2015

Collaborator

Small nits, LFAD otherwise.

Collaborator

krnowak commented Nov 27, 2015

Small nits, LFAD otherwise.

blixtra added some commits Oct 13, 2015

rkt: Create new OptionList type for csv flags
Adds a reusable type for flags that accept csv values. The user can
specify the list of permissible options as well as the default options.
Currently used in the 'image list' list command.
rkt: reuse field names thru formatters
This commit attempts to reuse the field names by modifying the names
appropriately via simple formatting functions. This reduces the amount
of changes needed to modify field names.

It also renames the lastusedtime header and variables to lastused.
stage0: break-up --insecure-skip-verify
Until now security features were either all on or all off. This commit
provides a means to turn specific security features on or off. By
default security features are all active.

This means that we've replaced the --insecure-skip-verify flag with one
called --insecure-options. This flag accepts a comma separated list of
features to turn off. The options that are currently accepted are
'none', 'image', 'tls', and 'all'. 'none' is the default and 'all' will
give one the previous behaviour from --insecure-skip-verify.

A BitFlags type was created to accept this list and provides a means to
check if a feature is active or not. For convenience, wrapper methods
are provided to query a feature's state; SkipImageCheck(),
SkipTlsCheck(), SkipFingerprintCheck(), SkipAllSecurityChecks(), and
SkipAnySecurityChecks().

Skipping the fingerprint review in `rkt trust` has also been moved to a
flag specific to the `trust` subcommand instead of being included in the
global --insecure-options flag.
Docs: add Global Options documentation
There was no documentation outside the command about the Global Options.
The commit adds that documentation to the commands.md command overview page.
Godeps: update cobra & pflags for new features
Cobra now provides suggestions when commands are mistyped. pflags now
allows for hiding flags from help output.

This also requires adding new dependencies on blackfriday, shurcooL &
go-md2man.
rkt: make --insecure-skip-verify == --insecure-options=all
It also marks --insecure-skip-verify as deprecated and hidden, meaning
the user will get a warning directing them to --insecure-options and
hide the flag from help output.

krnowak added a commit that referenced this pull request Nov 27, 2015

Merge pull request #1738 from kinvolk/stage0-break-apart-insecure-ski…
…p-verify

stage0 break apart --insecure-skip-verify

@krnowak krnowak merged commit b4984d1 into rkt:master Nov 27, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details

@achanda achanda referenced this pull request Dec 25, 2015

Merged

Update the insecure flag #633

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment