Skip to content

github-upload-action-secrets: support for ssh-keygen and deploy keys#2192

Merged
allisonkarlitskaya merged 6 commits intomainfrom
deploy-key-upload
Jul 9, 2021
Merged

github-upload-action-secrets: support for ssh-keygen and deploy keys#2192
allisonkarlitskaya merged 6 commits intomainfrom
deploy-key-upload

Conversation

@allisonkarlitskaya
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Member Author

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

Note: I considered to do things in this order:

  1. upload new deploy key
  2. update secret
  3. delete old deploy keys

So that any uses in the middle would work, but this is silly, and also pointless, because if a workflow is already running, it will keep using the old key...

One outstanding thing I want to make very very sure of is that the method that we're using to generate the key is getting high-quality random data.

@allisonkarlitskaya allisonkarlitskaya force-pushed the deploy-key-upload branch 3 times, most recently from 88b75b4 to a7bcfdb Compare July 8, 2021 10:03
@allisonkarlitskaya
Copy link
Copy Markdown
Member Author

Here's a working use of the automatically-generated/uploaded node-cache key: cockpit-project/cockpit#16068

Comment thread github-upload-action-secrets
Copy link
Copy Markdown
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Nice! With such a script and then collecting all the concrete cases in an Ansible playbook in cockpituous, my "codify/automate knowledge" concern will be completely addressed. Thanks!

Comment thread github-upload-action-secrets
Comment thread github-upload-action-secrets
Comment thread github-upload-action-secrets Outdated
Comment thread github-upload-action-secrets Outdated
Comment thread github-upload-action-secrets Outdated
Comment thread github-upload-action-secrets
Comment thread github-upload-action-secrets Outdated
@allisonkarlitskaya
Copy link
Copy Markdown
Member Author

Ok, so, choose your poison:

    key = Ed25519PrivateKey.generate()
    public = key.public_key().public_bytes(Encoding.OpenSSH, PublicFormat.OpenSSH)
    private = key.private_bytes(Encoding.PEM, PrivateFormat.OpenSSH, NoEncryption())

or

with tempfile.TemporaryDirectory() as dir:
    key=f'{dir}/link-to-fd3-and-fd4'

    yes_r, yes_w = os.pipe()
    priv_r, priv_w = os.pipe()
    publ_r, publ_w = os.pipe()

    try:
        os.symlink(f'/proc/self/fd/{priv_w}', f'{key}')
        os.symlink(f'/proc/self/fd/{publ_w}', f'{key}.pub')

        ssh_keygen = subprocess.Popen(
            ['ssh-keygen', '-t', 'ed25519', '-N', '', '-q', '-f', key],
            stdin=yes_r, stdout=open('/dev/null', 'w'),
            pass_fds = (publ_w, priv_w))
            
        os.write(yes_w, b'y\n')
        public = os.read(publ_r, 1024)
        private = os.read(priv_r, 1024)

    finally:
        for fd in (yes_r, yes_w, priv_r, priv_w, publ_r, publ_w):
            os.close(fd)

or

    with tempfile.TemporaryDirectory() as dir:
        key = f'{dir}/link-to-fd3-and-fd4'

        os.symlink(f'/proc/self/fd/3', f'{key}')
        os.symlink(f'/proc/self/fd/4', f'{key}.pub')
        process = subprocess.Popen(f'ssh-keygen -t ed25519 -N "" -q -f {key} 3>&1 4>&2 &>/dev/null', shell=True,
                                   stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
        private, public = process.communicate(b'y\n')

My personal preference is the pure library approach, but I understand that you have reservations about that. Second place is the "shell abuse" approach, I guess? Lucky that we have public/private and also stdout/stderr. Seems that the number 2 is popular!

@martinpitt
Copy link
Copy Markdown
Member

@allisonkarlitskaya Thanks for working these out! If nothing else, that's a nice reference to point to in the future. Undoubtedly 1 is the nicest (my previous review was blurred by that encrypt() mis-reading), 2 "OMG please no", and I find 3 actually quite clever. Not too big to understand/read, and doesn't require thinking about crypto or keeping up with SSH key format changes (which did happen not too long ago. So I feel like 2 > 1 > 3, but I would not veto 1 either -- after all, these are not long-lived, so if they stop working because reasons, we can always adjust the implementation.

@allisonkarlitskaya allisonkarlitskaya force-pushed the deploy-key-upload branch 2 times, most recently from 8b3b90e to c51fe38 Compare July 8, 2021 19:54
Copy link
Copy Markdown
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

One tiny rebase error, but this looks good. Many thanks for figuring this out! 💯

Comment thread github-upload-action-secrets
Comment thread github-upload-action-secrets Outdated
Make sure --receiver refers to a valid org or repository.
Get rid of `-d` option as an alias for `--dry-run` and use `-n`,
instead.
Add new --ssh-keygen and --directory options, exactly one of which must
be specified.

--directory gets you the old functionality of uploading a directory
worth of files as named secrets.

--ssh-keygen is a new feature which generates an SSH keypair and
directly uploads the private half as a secret (with the given name),
with the public key printed to stdout.
When used in combination with --ssh-keygen, instead of printing it to
stdout, upload the public half of the generated key to be used as a
deploy key on the named repository.
The old name was much too long, and it wasn't 100% accurate, since we
can now also upload to environments.
@allisonkarlitskaya allisonkarlitskaya merged commit 3fe38b8 into main Jul 9, 2021
@allisonkarlitskaya allisonkarlitskaya deleted the deploy-key-upload branch July 9, 2021 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants