Skip to content
This repository was archived by the owner on Oct 10, 2020. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Atomic/atomic.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ def __init__(self):
self.useTTY = True
self.syscontainers = SystemContainers()
self.run_opts = None
self.atomic_config = util.get_atomic_config()

def __enter__(self):
return self
Expand Down
129 changes: 129 additions & 0 deletions Atomic/sign.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
from . import util
from . import Atomic
import os
import tempfile
from .atomic import AtomicError
import re


ATOMIC_CONFIG = util.get_atomic_config()

def cli(subparser):
# atomic sign
signer = ATOMIC_CONFIG.get('default_signer', None)
signp = subparser.add_parser("sign",
help="Sign an image",
epilog="Create a signature for an image which can be "
"used later to verify it.")
signp.set_defaults(_class=Sign, func="sign")
signp.add_argument("images", nargs="*", help=_("images to sign"))
signp.add_argument("--signed-by", dest="sign_key", default=signer,
help=_("Name of the signing key. Currently %s, "
"default can be defined in /etc/atomic.conf" % signer))
signp.add_argument("-o", dest="output", default=None,
help=_("The filename of the signature"))


class Sign(Atomic):
def sign(self):
# TODO
# Atomic is run as sudo. Should we work around that?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t understand what this comment is trying to say. Is atomic(1) always run through sudo? (Probably not.) Does atomic sign always need to be run through sudo? (That would be surprising to me.)

Assuming “as sudo” means “as root”, this comment should say why is that needed so that it is clearer what to work around.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think the comment should say if atomic is run as root, nothing to do with sudo. If run as root we could try to figure out the homedir of the user running the command. SUDO_USER and /proc/self/login_uid, are good indicators of the users UID.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rhatdan would you be comfy tabling that part and circling back to it after the fact?

Copy link
Member

Choose a reason for hiding this comment

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

Yes table it.


if self.args.debug:
util.write_out(str(self.args))

signature_path = util.get_local_signature_path(self.atomic_config)

signer = self.args.sign_key

for sign_image in self.args.images:
remote_inspect_info = util.skopeo_inspect("docker://{}".format(sign_image))
manifest = util.skopeo_inspect('docker://{}'.format(sign_image), args=['--raw'], return_json=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

One line uses +, the line immediately below uses '{}'.format. That doesn’t really hurt but having both consistent would make it clearer that nothing is going on here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

try:
manifest_file = tempfile.NamedTemporaryFile(mode="wb", delete=False)
manifest_file.write(manifest)
manifest_file.close()
manifest_hash = str(util.skopeo_manifest_digest(manifest_file.name))

expanded_image_name = str(remote_inspect_info['Name'])
sigstore_path = "{}/{}/{}@{}".format(signature_path, os.path.dirname(expanded_image_name),
os.path.basename(expanded_image_name), manifest_hash)
self.make_sig_dirs(sigstore_path)
sig_name = self.args.output if self.args.output is not None else self.get_sig_name(sigstore_path)
fq_sig_path = os.path.join(sigstore_path, sig_name)
if os.path.exists(fq_sig_path):
raise ValueError("The signature {} already exists. If you wish to "
"overwrite it, please delete this file first")

util.skopeo_standalone_sign(expanded_image_name, manifest_file.name,
self.get_fingerprint(signer), fq_sig_path)
util.write_out("Created: {}".format(fq_sig_path))

finally:
os.remove(manifest_file.name)

def check_input_validity(self):
try:
for image in self.args.images:
self._is_image(image)
except AtomicError:
raise ValueError("{} is not a valid image".format(image))

@staticmethod
def get_fingerprint(signer):
cmd = ['gpg', '--no-permission-warning', '--with-colons', '--fingerprint', signer]
return_code, stdout, stderr = util.subp(cmd, newline=True)
if return_code is not 0:
raise ValueError(stderr)
for line in stdout.splitlines():
if line.startswith('fpr:'):
return line.split(":")[9]

@staticmethod
def make_sig_dirs(sig_path):
if not os.path.exists(sig_path):
# TODO
# perhaps revisit directory permissions
# when complete use-cases are known
os.makedirs(sig_path, '0777')

@staticmethod
def get_sig_name2(sig_path):
def missing_ints(aoi):
# Returns a list of integers in range
start, end = 1, max(aoi) + 1
if start == end and start is not 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

start is always 1 per the line above.

Copy link
Member Author

Choose a reason for hiding this comment

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

resolved by a different solution.

start = 1
_diff = sorted(set(range(start, end)).difference(aoi))
if len(_diff) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

This can never be true because end==max(aoi)+1 is not a member of aoi.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, end is exclusive. Never mind.

Copy link
Member Author

Choose a reason for hiding this comment

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

resolved by a different solution.

return end
else:
return min(_diff)

sigs = []
for sig in os.listdir(sig_path):
if re.match(r"signature-\b[0-9]+\b(?!\.[0-9])", sig):
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this supposed to match? This seems like an attempt to anchor the end of the string, but what is that \. doing in there at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

it matches signature-[positive_integer]

Copy link
Member Author

Choose a reason for hiding this comment

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

resolved by a different solution.

sigs.append(int(sig.replace("signature-", "")))

sigs.sort()
if len(sigs) == 0:
return "signature-1"
# In the event signature-0 exists
if sigs[0] == 0:
del sigs[0]
missing = missing_ints(sigs)
if missing == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICS this can never happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

resolved by a different solution.

sig_int = max(sigs) + 1
else:
sig_int = missing
return "signature-{}".format(sig_int)

@staticmethod
def get_sig_name(sig_path):
sig_files = set(os.listdir(sig_path))
sig_int = 1
while True:
name = "signature-{}".format(sig_int)
if name not in sig_files:
return name
sig_int += 1
44 changes: 39 additions & 5 deletions Atomic/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,13 @@ def image_by_name(img_name, images=None):
return valid_images


def subp(cmd, cwd=None):
def subp(cmd, cwd=None, newline=False):
# Run a command as a subprocess.
# Return a triple of return code, standard out, standard err.
proc = subprocess.Popen(cmd, cwd=cwd,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE, close_fds=True)
stderr=subprocess.PIPE, close_fds=True,
universal_newlines=newline)
out, err = proc.communicate()
return ReturnTuple(proc.returncode, stdout=out, stderr=err)

Expand Down Expand Up @@ -246,7 +247,7 @@ def urllib3_disable_warnings():
if hasattr(urllib3, 'disable_warnings'):
urllib3.disable_warnings()

def skopeo_inspect(image, args=None):
def skopeo_inspect(image, args=None, return_json=True, newline=False):
if not args:
args=[]

Expand All @@ -257,15 +258,18 @@ def skopeo_inspect(image, args=None):

cmd = ['skopeo', 'inspect'] + args + [image]
try:
results = subp(cmd)
results = subp(cmd, newline=newline)
except OSError:
raise ValueError("skopeo must be installed to perform remote inspections")
if results.return_code is not 0:
# Need to check if we are dealing with a v1 registry
check_v1_registry(image)
raise ValueError("Unable to interact with this registry: {}".format(results.stderr))
else:
return json.loads(results.stdout.decode('utf-8'))
if return_json:
return json.loads(results.stdout.decode('utf-8'))
else:
return results.stdout
Copy link
Contributor

@mtrmac mtrmac Aug 26, 2016

Choose a reason for hiding this comment

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

A single function returning two so wildly different types? If anybody ever tried to do static analysis of Python, this would throw if off completely.

*shrug* not my codebase, but personally I’d prefer a separate skopeo_inspect_raw function (which could in turn hardcode much more). Or, perhaps, just inline the util.subp call into sign.py.



def skopeo_delete(image, args=None):
Expand Down Expand Up @@ -318,6 +322,23 @@ def skopeo_layers(image, args=None, layers=None):
shutil.rmtree(temp_dir)
return temp_dir

def skopeo_standalone_sign(image, manifest_file_name, fingerprint, signature_path):
cmd = ['skopeo', 'standalone-sign', manifest_file_name,
image, fingerprint, "-o", signature_path]
try:
results = subp(cmd)
except Exception as e: # pylint: disable=broad-except
raise ValueError(e)
if results.return_code is not 0:
raise ValueError(results.stderr)

def skopeo_manifest_digest(manifest_file):
cmd = ['skopeo', 'manifest-digest', manifest_file]
try:
results = subp(cmd)
except Exception: #pylint: disable=broad-except
raise ValueError(results.stderr)
return results.stdout.rstrip().decode()

def check_v1_registry(image):
# Skopeo cannot interact with a v1 registry
Expand Down Expand Up @@ -354,7 +375,12 @@ def get_atomic_config_item(config_items, atomic_config=None):
Lookup and return the atomic configuration file value
for a given structure. Returns None if the option
cannot be found.

** config_items must be a list!
"""

assert isinstance(config_items, list)

def _recursive_get(atomic_config, items):
yaml_struct = atomic_config
try:
Expand Down Expand Up @@ -498,3 +524,11 @@ def validate_manifest(spec, img_rootfs=None, img_tar=None, keywords=""):
if keywords:
cmd += ['-k',keywords]
return subp(cmd)

def get_local_signature_path(atomic_conf):
# If signature path is defined, get it; else return default
signature_path = get_atomic_config_item(['default-sigstore-path'], atomic_config=atomic_conf)
if signature_path is None:
signature_path = '/var/lib/atomic/sigstore'
return signature_path
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to make this consistent with https://github.com/mtrmac/image/blob/docker-lookaside/docker/lookaside.go#L102 :

  • Using URLs
  • "registries"."" instead of default-sigstore-path (though that might be by changing containers/image instead)
  • sigstore-write values
  • No /etc/pki/containers default I think… A single sigstore for all of the internet really isn’t likely to be useful. For any external public repositories, there would be a read-only sigstore via HTTP. Read-write would be only the locally created/managed repositories (mydepartment.mycompany.com/registry) or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

… and the per-registry/per-namespace/per-repository lookup from lookaside.go is necessary as well; we need various ISVs who publish to Docker hub to be able to maintain their own sigstore servers.


2 changes: 2 additions & 0 deletions atomic
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ from Atomic import push
from Atomic import stop
from Atomic import uninstall
from Atomic import update
from Atomic import sign
from Atomic.util import write_err, NoDockerDaemon
import traceback
from Atomic.mount import MountError
Expand Down Expand Up @@ -144,6 +145,7 @@ def create_parser(help_text):
pull.cli(subparser)
run.cli(subparser)
scan.cli(subparser)
sign.cli(subparser)
stop.cli(subparser)
storage.cli(subparser)
top.cli(subparser)
Expand Down
55 changes: 55 additions & 0 deletions docs/atomic-sign.1.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
% ATOMIC(1) Atomic Man Pages
% Brent Baude
% August 2016
# NAME
atomic-sign- Create a signature for an image

**WARNING**

Only use **atomic sign** if you trust the remote registry which contains the image
(preferably by being the only administrator of it).


# SYNOPSIS
**atomic sign**
[**-h**|**--help**]

[**-o**, **--output**]
[**--sign_key**]
[ image ... ]

# DESCRIPTION
**atomic sign** will create a local signature for one or more local images that have
been pulled from a registry. Unless overridden, the signature will end up in the
the default storage location (/var/lib/atomic/containers) for signatures. A different
default location can be defined in /etc/atomic.conf with the key **default-sigstore-path**.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to be expanded for the various more strictly namespaced paths, per lookaside.go or similar.


# OPTIONS
**-h** **--help**
Print usage statement.

**-o** **--output**
Assign a specific signature file name; otherwise, the file name is generated.


**--signed-by**
Override the default identity of the signature. You can define a default in /etc/atomic.conf
with the key **default_signer**.

Copy link
Member

Choose a reason for hiding this comment

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

Where is the default signature defined?

Copy link
Member

Choose a reason for hiding this comment

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

Really need a couple of examples here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added location information and information around defaults. Added two additional signing examples.


# EXAMPLES
Sign the foobar image from privateregistry.example.com

atomic sign privateregistry.example.com/foobar

Sign the foobar image with a specific signature name.

atomic sign -o foobar.sig privateregistry.example.com

Sign the busybox image with the identify of foo@bar.com

atomic --signed-by foo@bar.com privateregistry.example.com

Copy link
Contributor

Choose a reason for hiding this comment

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

Per the WARNING suggested above, all of this should use privateregistry.example.com instead of docker.io.

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea


# HISTORY
Initial revision by Brent Baude (bbaude at redhat dot com) August 2016