Skip to content

Add support for multiple deploy key and GitHub Wiki#276

Closed
ylemkimon wants to merge 3 commits into
drdoctr:masterfrom
ylemkimon:wiki
Closed

Add support for multiple deploy key and GitHub Wiki#276
ylemkimon wants to merge 3 commits into
drdoctr:masterfrom
ylemkimon:wiki

Conversation

@ylemkimon

@ylemkimon ylemkimon commented Nov 10, 2017

Copy link
Copy Markdown
Contributor
  • Add support for multiple deploy key encryption key. Automatically determines deploy key encryption key environment variable name and filename from deploy key repo name: Fixes Multiple deploy keys #242.
  • Add support for deployment to GitHub wiki: Fixes Support GitHub wikis #275.

@asmeurer

Copy link
Copy Markdown
Member

Thanks. I'll try to take a look soon. Is the multiple deploy key thing required for the wiki?

@asmeurer

Copy link
Copy Markdown
Member

Oh right I guess it would be required for SymPy, since we already use doctr for the docs, which are in a separate repo.

@ylemkimon

ylemkimon commented Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

@asmeurer Or, now I think about it, maybe we can put it to the other build than Sphinx build(TEST_SPHINX) in SymPy.

@asmeurer

Copy link
Copy Markdown
Member

Indeed. Do per-matrix variables already work in the existing doctr? I don't think I've tested this.

@ylemkimon

ylemkimon commented Nov 11, 2017

Copy link
Copy Markdown
Contributor Author

@asmeurer In matrix-include-env, it works fine. However, in env-matrix, it has to be encrypted with the plain variables like travis-encrypt SUPER_SECRET="SECRET";SPLIT="1/4";TEST_SYMPY="true". Travis will only recognize SUPER_SECRET as environment variable, but as it is processed as bash statement, other environment variables will be set.

Edit: Second option will not work with PR builds.

@asmeurer

Copy link
Copy Markdown
Member

Wait it really lets you encrypt more than one environment variable just by putting semicolons in them?

@ylemkimon

ylemkimon commented Nov 11, 2017

Copy link
Copy Markdown
Contributor Author

@asmeurer Yes, I think spaces will also work if we directly encrypt it. However, Travis doesn't recognize it as secure variable, so it does not mask it. Also, since it is encrypted, it will not be available to PR builds. For encrypting multiple global variables, we can do:

env:
  global:
    - secure: [encrypted 1]
    - secure: [encrypted 2]

Comment thread doctr/local.py
if wiki and not private:
# private wiki needs authentication, so skip check for existence
p = subprocess.run(['git', 'ls-remote', '-h', 'https://github.com/{user}/{repo}.wiki'.format(
user=user, repo=repo)], stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=False)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can do:

p = subprocess.run(['git', 'ls-remote', '-h', 'https://{auth}@github.com/{user}/{repo}.wiki'.format(
    auth=base64.urlsafe_b64encode(bytes(auth.username + ':' + auth.password, 'utf8')).decode(),
    user=user, repo=repo)], stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=False)

@asmeurer

Copy link
Copy Markdown
Member

OK, I did some refreshing on #242. I think if we are going to do refactoring there, we should go full option 3 from that issue, that is, storing the full SSH key in the environment variable (automatically named DOCTR_DEPLOY_KEY_ORG_REPO or something like that). I don't know if we'll need the key flag that's been added here after that. I can try to do some work on this this week, unless you want to work on it.

By the way, the semicolon thing isn't useful because we have to encrypt the keys separately (they are generated by different doctr configure runs in general). The matrix thing would be nice, but I couldn't figure out how to make it work (see https://stackoverflow.com/questions/45993334/multiple-values-of-a-secure-encrypted-environment-variable-in-travis-yml). At any rate, that change wouldn't really be relevant to anything other than the documentation and help output of doctr configure. The variable itself would still be set just the same to doctr deploy. The main difference is that it would be more secure to have the secure variable only available for the specific builds that need it.

@ylemkimon

ylemkimon commented Nov 13, 2017

Copy link
Copy Markdown
Contributor Author

I can try to do some work on this this week, unless you want to work on it.

If you don't mind, I'd like to work on this.

The matrix thing would be nice, but I couldn't figure out how to make it work

We can include it in the matrix like (excerpt from sympy/sympy):

matrix:
  include:
    - python: 3.6
      env:
        - TEST_ASCII="true"
        - TEST_OPT_DEPENDENCY="..."
        - secure: [encrypted 1]

    - python: 3.6
      env:
        - TEST_SPHINX="true"
        - FASTCACHE="false"
        - secure: [encrypted 2]

I think this probably work in most use cases, since usually deployment build of documentation is a separate build.

@ylemkimon

Copy link
Copy Markdown
Contributor Author

@asmeurer I'm afraid that Travis is using RSA-2048 PKCS #1 for encrypting variables and it can encrypt maximum 245 bytes(RFC 2313). So we cannot remove Fernet encryption and it will be just including encrypted deploy key(.enc file) in travis.yml which, I think, is bad for readability of travis.yml.

@asmeurer

Copy link
Copy Markdown
Member

I see. So that explains why it's always been done this way.

So we should just use DOCTR_DEPLOY_ENCRYPTION_KEY_ORG_REPO, and the default file name should probably encorporate the org/repo as well. And we need to keep backwards compatibility with the current scheme.

Automatically determine deploy key encryption key environment variable
name and file name from deploy key repo name.
@ylemkimon

Copy link
Copy Markdown
Contributor Author

@asmeurer Added new commits. It automatically determine deploy key encryption key environment variable name and filename from deploy key repo name.

@asmeurer

Copy link
Copy Markdown
Member

OK, I finally have time to work on this.

In order to test this, I've needed to push it up to the main repo. I made a new PR #280. I'm closing this for now. If you want to make more changes you can PR them to the wiki branch.

@asmeurer asmeurer closed this Nov 18, 2017
@ylemkimon

Copy link
Copy Markdown
Contributor Author

@asmeurer It seems it is possible to set long secure environment variables in Travis repository setting, but I haven't tested it.

@asmeurer

Copy link
Copy Markdown
Member

Is there an API for it?

@ylemkimon

Copy link
Copy Markdown
Contributor Author

@asmeurer Yes and it seems that #176 implements it.

@ylemkimon

Copy link
Copy Markdown
Contributor Author

The downside is that it is not version controlled and it can be only accessed by the repo's owner.

@asmeurer

Copy link
Copy Markdown
Member

So I guess the current method is worth keeping?

@gforsyth what do you think?

@asmeurer

Copy link
Copy Markdown
Member

can be only accessed by the repo's owner.

Like you need GitHub owner permissions, not just push access?

Currently to add a deploy key to a GitHub repo you need push access (I believe, or does it require full admin?).

@ylemkimon

Copy link
Copy Markdown
Contributor Author

@asmeurer It needs full admin(owner) access to add a deploy key, too, sorry. I was thinking more like, we or users need another layer of encryption to configure with --no-upload-key and setup Doctr for a repo you don’t have admin access to, or owner has to doctr configure him/herself.

@asmeurer

Copy link
Copy Markdown
Member

So it sounds like the current scheme is useful enough to keep around, even if we implement direct key uploading.

If we do implement direct key uploading, which should become the default? I'm not sure. Unless there is a major advantage or disadvantage for one or the other that hasn't been pointed out yet, I would just make doctr configure ask.

@asmeurer

Copy link
Copy Markdown
Member

In other words, unless there are objections, I think it's fine to do a doctr release, and effectively cement the new multiple repo scheme (once we release it, we need to support it indefinitely).

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