Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

File permissions validations #355

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

File permissions validations #355

wants to merge 2 commits into from

Conversation

JoshVanL
Copy link
Contributor

@JoshVanL JoshVanL commented Jun 29, 2018 •
edited

What this PR does / why we need it:
Adds validations to ensure 600 permissions on id_rsa, ssh_config and vault_root_token

fixes #170

/assign @charlieegan3

NONE

@jetstack-bot jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 29, 2018
@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattbates

Assign the PR to them by writing /assign @mattbates in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@charlieegan3
Copy link
Contributor

I've looked over this and the code changes look fine.

When I have the incorrect permissions set now I get the following error:

FATA[0005] Tarmak exited with an error: failed to validate tarmak: 2 errors occurred:

* vault root token file '/home/charlieegan3/.tarmak/charlie/vault_root_token' does not match permissions (0600): -r--------
* '/home/charlieegan3/.tarmak/charlie/id_rsa' does not match permissions (0600): -r--------  error="failed to validate tarmak: 2 errors occurred:\n\n* vault root token file '/home/charlieegan3/.tarmak/charlie/vault_root_token' does not match permissions (0600): -r--------\n* '/home/charlieegan3/.tarmak/charlie/id_rsa' does not match permissions (0600): -r--------"

Clearly a huge improvement on the output in #170.

@charlieegan3
Copy link
Contributor

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2018
@charlieegan3
Copy link
Contributor

/unassign
/assign @simonswine

simonswine
simonswine suggested changes Jul 6, 2018
View reviewed changes
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a few comments from my side

/assign @JoshVanL
/unassign

pkg/tarmak/vault/vault.go Outdated
return fmt.Errorf("failed to get vault root token '%s' file stat: %v", path, err)
}

if f.Mode() != os.FileMode(0600) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be only checking if others and group are having a zero like that

(f.Mode() & 0077) == 0

pkg/tarmak/ssh/ssh.go Outdated
err := fmt.Errorf("'%s' does not match permissions (0600): %v", path, f.Mode())
result = multierror.Append(result, err)
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be only checking if others and group are having a zero like that

(f.Mode() & 0077) == 0

pkg/tarmak/tarmak.go
@@ -281,20 +281,21 @@ func (t *Tarmak) Version() string {
}

func (t *Tarmak) Validate() error {
var err error
var result error
var result *multierror.Error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this whole method should just be calling to validateSSHSetup() or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too sure what you mean by this. tarmak.Validate() should only call to validateSSHSetup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonswine

@jetstack-bot jetstack-bot assigned JoshVanL and unassigned simonswine Jul 6, 2018
@jetstack-bot jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2018
@jetstack-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@JoshVanL
Copy link
Contributor Author

@simonswine

@JoshVanL
Copy link
Contributor Author

/assign @simonswine

@JoshVanL
Copy link
Contributor Author

/unassign

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2018
@jetstack-bot
Copy link
Collaborator

@JoshVanL: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jetstack-bot jetstack-bot added the dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. label Nov 9, 2018
@jetstack-bot
Copy link
Collaborator

@JoshVanL: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
tarmak-puppet-module-tarmak-acceptance-1-14-centos d5caa32 link /test puppet-tarmak-acceptance-centos v1.14

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

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

@simonswine simonswine simonswine requested changes

Assignees

@simonswine simonswine

Labels
dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Better explain incorrect keyfile permissions
4 participants
@JoshVanL @jetstack-bot @charlieegan3 @simonswine