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

Massive rewrite of SSH section for 2025's authentication changes #266

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

obilaniu
Copy link
Contributor

@obilaniu obilaniu commented Mar 3, 2025

No description provided.

@obilaniu obilaniu force-pushed the ssh-keys branch 4 times, most recently from 72d7516 to d3f6b5a Compare March 3, 2025 08:24
@btravouillon btravouillon requested review from ahmam and milaflq March 3, 2025 23:50
Copy link
Collaborator

@btravouillon btravouillon left a comment

Choose a reason for hiding this comment

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

Impressive work! ❤️
Some comments to open discussion and one request to move one section above the others.

The login nodes support the following authentication mechanisms:
``publickey,keyboard-interactive``. If you would like to set an entry in your
``.ssh/config`` file, please use the following recommendation:
Logging in with SSH
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move this section before "mila init". Plain old SSH is the easiest and fastest way to connect to the cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok... that slightly contradicts my description of mila init as doing everything for you. And this SSH config block does look slightly intimidating, and doesn't include the mila-cpu entry that mila init generates and that gets mentioned a lot elsewhere. @lebrice?

Copy link
Collaborator

@lebrice lebrice Mar 4, 2025

Choose a reason for hiding this comment

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

Hmm. With these changes, it's still unclear to me to what extent the "role" of mila init should be affected.
Here's exactly what running mila init does at the moment:

  • Asks the user for their Mila / DRAC usernames
  • Populates the SSH configuration file
  • Generates an ssh keypair if not present
  • Runs ssh-copy-id mila and optionally does the same for DRAC
  • Generates an ssh keypair without passphrase on the login nodes, and adds it to authorized keys (to allow connecting to the compute nodes from the login node)
  • When launched from WSL, also attempts to configure the Windows SSH configuration and keypair.
  • Adds useful default settings for connecting to the cluster with Visual Studio code in the users's settings.json file.

I guess following these changes, we can either try to send the form programmatically (which might be difficult given Google Auth + 2FA) or just print a link to the form, and display the info that the user would then have to copy-paste into the form.

What do you think @breuleux @obilaniu @btravouillon ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if mila init can do everything for you, the simplest command to access the cluster once the access is granted is plain old SSH. First, we must document how to access the cluster with a simple SSH command. This is how the documentation is written today. Just move the mila init section below. It does not dismiss any additional information on how to configure the ssh client (~/.ssh/config) nor how to use mila init. But I prefer to get the simplest command first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reordered to put manual SSH configuration first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you double check? I'm reviewing right now and the "Logging in with SSH" is still after "mila init"/

The login nodes support the following authentication mechanisms:
``publickey,keyboard-interactive``. If you would like to set an entry in your
``.ssh/config`` file, please use the following recommendation:
Logging in with SSH
Copy link
Collaborator

@lebrice lebrice Mar 4, 2025

Choose a reason for hiding this comment

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

Hmm. With these changes, it's still unclear to me to what extent the "role" of mila init should be affected.
Here's exactly what running mila init does at the moment:

  • Asks the user for their Mila / DRAC usernames
  • Populates the SSH configuration file
  • Generates an ssh keypair if not present
  • Runs ssh-copy-id mila and optionally does the same for DRAC
  • Generates an ssh keypair without passphrase on the login nodes, and adds it to authorized keys (to allow connecting to the compute nodes from the login node)
  • When launched from WSL, also attempts to configure the Windows SSH configuration and keypair.
  • Adds useful default settings for connecting to the cluster with Visual Studio code in the users's settings.json file.

I guess following these changes, we can either try to send the form programmatically (which might be difficult given Google Auth + 2FA) or just print a link to the form, and display the info that the user would then have to copy-paste into the form.

What do you think @breuleux @obilaniu @btravouillon ?

@obilaniu obilaniu force-pushed the ssh-keys branch 3 times, most recently from b31bf14 to a9fbde5 Compare March 4, 2025 16:34
@btravouillon btravouillon self-requested a review March 11, 2025 14: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