Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSH passphrases support #70

Merged
merged 10 commits into from
Aug 7, 2021
Merged

SSH passphrases support #70

merged 10 commits into from
Aug 7, 2021

Conversation

Jelleas
Copy link
Contributor

@Jelleas Jelleas commented Aug 2, 2021

A quick draft for supporting SSH keys protected by passphrases. The student is prompted for their passphrase if needed, and they will see ***** stars for each letter entered, just like with passwords previously. The passphrase is kept in memory with the User and used to:

  1. check that authentication is possible
  2. clone their student repo
  3. push the commit

I refactored all the authentication related code to authentication.py, but this could use some more polish and is by no means clean just yet.

Here's what a run looks like with debug log level:

~/ $ check50 cs50/problems/2021/x/hello --log-level debug
Connecting...
(DEBUG) 37c318950c7c6a57ce7ebe0b4de74af6f440ff5b        refs/heads/2021/x
bff6780da5a5980fbfb5578e20e38f69b1d2c42c        refs/heads/hello
Authenticating...
Enter passphrase for SSH key: *************************
Verifying...
(INFO) git clone --bare --single-branch ssh://git@ssh.github.com:443/me50/Jelleas .git --branch cs50/problems/2021/x/hello
(DEBUG) Cloning into bare repository '.git'...
(DEBUG) Enter passphrase for key '/home/jelle/.ssh/id_rsa':
(DEBUG) warning: Could not find remote branch cs50/problems/2021/x/hello to clone.
fatal: Remote branch cs50/problems/2021/x/hello not found in upstream origin
(DEBUG) git -C /tmp/tmpuxbohp6v  clone --bare --single-branch ssh://git@ssh.github.com:443/me50/Jelleas .git --branch cs50/problems/2021/x/hello exited with 128
(INFO) git clone --bare --single-branch ssh://git@ssh.github.com:443/me50/Jelleas .git
(DEBUG) Cloning into bare repository '.git'...
(DEBUG) Enter passphrase for key '/home/jelle/.ssh/id_rsa':
(DEBUG) remote: Enumerating objects: 6, done.
remote: Counting objects: 100% (5/5)
(DEBUG) remote: Counting objects: 100% (5/5), done.
remote: Compressing objects: 100% (3/3), done.
(DEBUG) remote: Total 6 (delta 1), reused 4 (delta 0), pack-reused 1
Receiving objects:  66% (4/6)
Receiving objects: 100% (6/6), done.)
(DEBUG) Resolving deltas:   0% (0/1)
Resolving deltas: 100% (1/1), done.)
Preparing...
(INFO) git config --bool core.bare false
(INFO) git config --path core.worktree /tmp/tmpuxbohp6v
(INFO) git checkout --force cs50/problems/2021/x/hello .gitattributes
(DEBUG) error: pathspec 'cs50/problems/2021/x/hello' did not match any file(s) known to git
error: pathspec '.gitattributes' did not match any file(s) known to git
(DEBUG) git -C /tmp/tmpuxbohp6v checkout --force cs50/problems/2021/x/hello .gitattributes exited with 1
(INFO) git config user.email Jelleas@users.noreply.github.com
(INFO) git config user.name Jelleas
(INFO) git symbolic-ref HEAD refs/heads/cs50/problems/2021/x/hello
(INFO) git add -f hello.c
Uploading...
(INFO) git commit -m 'automated commit by check50 [check50=True]' --allow-empty
(DEBUG) [cs50/problems/2021/x/hello (root-commit) 2aedd2c] automated commit by check50 [check50=True]
(DEBUG) 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 hello.c
(INFO) git push origin cs50/problems/2021/x/hello
(DEBUG) Enter passphrase for key '/home/jelle/.ssh/id_rsa':
(DEBUG) Enumerating objects: 3, done.
Counting objects: 100% (3/3), done.)
Writing objects: 100% (3/3), 235 bytes | 235.00 KiB/s, done.
Total 3 (delta 0), reused 1 (delta 0)
(DEBUG) remote:
remote: Create a pull request for 'cs50/problems/2021/x/hello' on GitHub by visiting:
remote:      https://github.com/me50/Jelleas/pull/new/cs50/problems/2021/x/hello
remote:
(DEBUG) To ssh://ssh.github.com:443/me50/Jelleas
 * [new branch]      cs50/problems/2021/x/hello -> cs50/problems/2021/x/hello
(INFO) git rev-parse HEAD
(DEBUG) 2aedd2c4d8817c270812afc2b38c5efc9d8876cd
Waiting for results...
(DEBUG) {'slug': 'cs50/problems/2021/x/hello', 'results': [{'cause': None, 'data': {}, 'dependency': None, 'description': 'hello.c exists', 'log': ['checking that hello.c exists...'], 'name': 'exists', 'passed': True}, {'cause': {'help': None, 'rationale': 'code failed to compile'}, 'data': {}, 'dependency': 'exists', 'description': 'hello.c compiles', 'log': ['running clang hello.c -o hello -std=c11 -ggdb -lm -lcs50...', "/usr/bin/ld: /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../x86_64-linux-gnu/crt1.o: in function `_start':", "(.text+0x24): undefined reference to `main'", 'clang-10: error: linker command failed with exit code 1 (use -v to see invocation)'], 'name': 'compiles', 'passed': False}, {'cause': {'rationale': "can't check until a frown turns upside down"}, 'data': {}, 'dependency': 'compiles', 'description': 'responds to name Emma', 'log': [], 'name': 'emma', 'passed': None}, {'cause': {'rationale': "can't check until a frown turns upside down"}, 'data': {}, 'dependency': 'compiles', 'description': 'responds to name Rodrigo', 'log': [], 'name': 'rodrigo', 'passed': None}], 'version': '3.3.0'}
Results for cs50/problems/2021/x/hello generated by check50 v3.3.0
:) hello.c exists
:( hello.c compiles
    code failed to compile
:| responds to name Emma
    can't check until a frown turns upside down
:| responds to name Rodrigo
    can't check until a frown turns upside down
To see the results in your browser go to https://submit.cs50.io/check50/98ef9412f8b3f0e80d87f0412968801f10b0c159

try:
state = State(child.expect([
"Permission denied",
"Hi (.+)! You've successfully authenticated",
Copy link
Member

Choose a reason for hiding this comment

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

Does git always use English, even if some locale-related env vars are present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Theoretically, git uses gettext to translate it's messages:
https://github.com/git/git/blob/e5a14ddd2d93da4d951fd63d4f78fe2410debe68/Documentation/RelNotes/1.7.9.txt#L13

In practice, I don't think we've encountered this issue just yet.

:return: None
:type: None
"""
api._run(f"git credential-cache --socket {_CREDENTIAL_SOCKET} exit")
Copy link
Member

Choose a reason for hiding this comment

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

Will this get run even if there's an exception elsewhere, just to ensure the cache is flushed?

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 does:

except BaseException:
# Some error occured while this context manager is active, best forget credentials.
logout()
raise

logout is also part of the external api and is what makes this work:

check50 --logout
submit50 --logout

@dmalan
Copy link
Member

dmalan commented Aug 2, 2021

BTW, @Jelleas, can we detect if a user is using a password and output a message like this?

You seem to be using your GitHub password to log in, but that's no longer possible. But you can still use check50 and submit50! See https://cs50.ly/github for instructions.

@Jelleas
Copy link
Contributor Author

Jelleas commented Aug 5, 2021

BTW, @Jelleas, can we detect if a user is using a password and output a message like this?

You seem to be using your GitHub password to log in, but that's no longer possible. But you can still use check50 and submit50! See https://cs50.ly/github for instructions.

I don't see any way to differentiate between using a PAT and a password just yet. But, that might change past Aug 13. For now I propose we show a warning like so when HTTPS authentication fails:

image

@brianyu28
Copy link
Member

Thanks, @Jelleas! Two things I noticed:

  1. If I hit Ctrl-C to exit check50 immediately when I'm prompted for a username, I see the warning message about using a GitHub password?

Screen Shot 2021-08-05 at 2 17 14 PM

  1. If I type in the wrong SSH key, I'm prompted to type in the SSH key two additional times, and then prompted for my GitHub username. Is it possible to detect that the password is incorrect the first time and let the user know if so?

Screen Shot 2021-08-05 at 2 20 57 PM

@Jelleas
Copy link
Contributor Author

Jelleas commented Aug 6, 2021

@brianyu28

  1. Good catch! I've separated the handling of Exceptions and BaseExceptions (SIGINT falls under the latter). Only "normal" exceptions will show that warning now, for instance if the username+password is incorrect. See fb74d79

  2. The number of times SSH will re-prompt is set by a configuration option somewhere. But it's easy enough to show custom messages on a re-prompt or when auth eventually fails (See e4a22c2). I've added the messages below:

image

@Jelleas
Copy link
Contributor Author

Jelleas commented Aug 6, 2021

Added the following preemptive warning as per request:

GitHub now requires that you use SSH or a personal access token instead of a password to log in, but you can still use check50 and submit50! See https://cs50.ly/github for instructions if you haven't already!

This is what the path of max warnings looks like now:

image

@dmalan
Copy link
Member

dmalan commented Aug 6, 2021

Hey, thanks so much, @Jelleas. For consistency with the SSH wording, should we perhaps reword prompts as these?

Enter username for GitHub:
Enter personal access token for GitHub:

That'll also make clearer that something has changed?

And can we remove the (WARN) prefix, just so it doesn't look like an (ongoing) error?

And, just to be sure, any changes still needed for submit50?

Thank you!!

@Jelleas
Copy link
Contributor Author

Jelleas commented Aug 7, 2021

Changed the phrasing and left out the WARNING prefix in 2bea4e4.

I figured printing is the cleanest solution here as alternatively the logger's Formatter needs to change to not include prefixes, but that's unwanted in most cases. Whereas printing here should have no downsides, besides now being unable to mute it through --log-level error. But I strongly doubt anyone will ever run --log-level error while using check50's remote operation anyway.

Quick side note, other colors are possible too. But I stuck with yellow as it stands out nicely.

image

@Jelleas Jelleas merged commit baf2494 into develop Aug 7, 2021
@Jelleas Jelleas deleted the passphrase branch August 7, 2021 11:53
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.

3 participants