Skip to content
This repository was archived by the owner on Oct 10, 2020. It is now read-only.

Inline pubkeys in policy.json#853

Closed
aweiteka wants to merge 2 commits intoprojectatomic:masterfrom
aweiteka:inline-pubkeys
Closed

Inline pubkeys in policy.json#853
aweiteka wants to merge 2 commits intoprojectatomic:masterfrom
aweiteka:inline-pubkeys

Conversation

@aweiteka
Copy link
Contributor

@aweiteka aweiteka commented Jan 27, 2017

Default is now to inline pubkeys as "keyData" strings inside policy.json, with option to reference as installed files ("keyPath") on host system, the previous behavior.

We also now support passing in a URL to a public key.

for **signedBy** type.

**-f**_**--file**
Reference pubkeys as files on host system instead of inlining pubkey data
Copy link
Member

Choose a reason for hiding this comment

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

Should this be public keys as files ...inlining public key data

Atomic/trust.py Outdated
showp.add_argument('-j', '--json', action='store_true', help="Output as json")
showp.set_defaults(_class=Trust, func="show")
addp.add_argument('-f', '--file', action='store_true',
help=_("Reference pubkeys as files on host system instead "
Copy link
Member

Choose a reason for hiding this comment

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

public keys?

Atomic/trust.py Outdated
self.atomic_config = util.get_atomic_config()

def add(self, registry=None, pubkeys=None, sigstore=None, sigstoretype=None, keytype=None, trust_type=None):
def add(self, registry=None, pubkeys=None, sigstore=None, sigstoretype=None, keytype=None, trust_type=None, pubkeys_file=False):
Copy link
Member

Choose a reason for hiding this comment

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

Is pubkeys_file a boolean or a path? Should we just check if path == None

Atomic/trust.py Outdated
:param keytype: string, "GPGKeys"
:param trust_type: string, one of "signedBy", "insecureAcceptAnything", "reject"
:param sigstore: string, URL of signature server
:param pubkeys_file: boolean, reference pubkeys as filepath
Copy link
Member

Choose a reason for hiding this comment

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

If it is a boolean it should not be called a _file.

@rhatdan
Copy link
Member

rhatdan commented Feb 2, 2017

You are still referencing pubkeys in the --help and man page for the user, I think we should spell it out for those cases. Also the CLI now has --pubkeys and --file? Is this confusing?

@aweiteka
Copy link
Contributor Author

aweiteka commented Feb 3, 2017

You are still referencing pubkeys in the --help and man page for the user, I think we should spell it out for those cases.

The option is pubkeys but I think I spell it out as "public keys" in the help/manpage. Do you want a longer option like --public-keys?

Also the CLI now has --pubkeys and --file? Is this confusing?

Yes, possibly. How can we make this less confusing? I considered --inline but the user would need to use --inline=False to change default. Not sure.

@rhatdan
Copy link
Member

rhatdan commented Feb 3, 2017

Lets discuss this together.

class Args():
def __init__(self):
self.debug = None
self.assumeyes = None
Copy link
Member

Choose a reason for hiding this comment

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

Why do I need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be called from Atomic/pull.py (the auto-discover trust workflow), in which case it fails to pass in global opts. @baude we discussed this. Not sure if there is a more elegant workaround. It works but does seem silly.

Copy link
Member

Choose a reason for hiding this comment

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

Ok

"Keys are parsed and encoded into policy.json. "
"May used multiple times to define multiple public keys. "
"File(s) must exist before using this command."))
commonp.add_argument("-f", "--pubkeysfile", nargs='?', default=[],
Copy link
Member

Choose a reason for hiding this comment

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

Shoudl --pubkeys and --pubkeysfile be mutually exlusive? Or would a user specify both on the same command line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible and valid to use both commands, although unexpected. No reason to restrict it, IMO.

Copy link
Member

Choose a reason for hiding this comment

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

ok

@rhatdan
Copy link
Member

rhatdan commented Feb 6, 2017

Tests are failing on assumeyes?

@aweiteka
Copy link
Contributor Author

aweiteka commented Feb 6, 2017

Fixed.

@rhatdan
Copy link
Member

rhatdan commented Feb 6, 2017

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit a296d6c has been approved by rhatdan

@cgwalters
Copy link
Member

@rh-atomic-bot r=rhatdan a296d6c

@rh-atomic-bot
Copy link

⌛ Testing commit a296d6c with merge ae8e281...

rh-atomic-bot pushed a commit that referenced this pull request Feb 6, 2017
Closes: #853
Approved by: rhatdan
@rh-atomic-bot
Copy link

☀️ Test successful - status-redhatci
Approved by: rhatdan
Pushing ae8e281 to master...

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants