Skip to content

Conversation

@krokicki
Copy link
Member

@krokicki krokicki commented Jan 7, 2026

This PR adds an SSH key manager to the profile menu in the upper right. This lets you create SSH keys and authorize them so that you can seamlessly move between cluster nodes. The main purpose for this is to allow easy on boarding for our Seqera Platform instance.

I tried to be careful about making backups of files in ~/.ssh before changing them, but would welcome feedback especially if there are any edge cases I didn't think of. If you are testing this, I would recommend making a backup of your ~/.ssh directory first, just in case.

@JaneliaSciComp/fileglancer @StephanPreibisch

@krokicki krokicki requested review from cgoina and neomorphic January 8, 2026 01:17
Copy link
Contributor

@mkitti mkitti left a comment

Choose a reason for hiding this comment

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

I think we should consider treating .ssh as read-only as much as possible. Keys generated by fileglancer could be stored elsewhere.

The main file that may need to be written to here might be .ssh/authorized_keys . We should consider if .ssh/authorized_keys2 could be used or if the sshd configuration could be modified to recognize an authorized_keys file exclusively managed by fileglancer.

Here is the documentation for OpenSSH sshd:
https://man.openbsd.org/sshd_config#AuthorizedKeysFile

@krokicki
Copy link
Member Author

krokicki commented Jan 13, 2026

I added a VITE_ENABLE_SSH_KEYS feature toggle, and enabled both this and VITE_ENABLE_TASKS in the CI build. I also modified the implementation to do as little as possible with ~/.ssh.

@krokicki
Copy link
Member Author

@mkitti As discussed, we agree about "treating .ssh as read-only as much as possible". To minimize complexity and because for now this feature has a very small user base, I've modified the GUI to do the most minimal thing possible to ~/.ssh, namely: if id_ed25519 does not exist, this tool can create one for you and add it to authorized_keys using cat. I've removed functionality around managing multiple keys, and all functionality that deletes keys. This minimizes risks to user's existing SSH keys and setups.

@mkitti
Copy link
Contributor

mkitti commented Jan 13, 2026

In a few places we still read the private key into resident memory of the Python process as an immutable Python string. This is not good. Core dumps of the Python process may contain the contents of the private key.

Rather than loading file contents into an immutable string, we should read the contents into a mutable bytearray and then explicitly overwrite the contents of the bytearray when the contents are no longer needed.

import os

key_path = "private_key.pem"
file_size = os.path.getsize(key_path)

# 1. Pre-allocate mutable memory
sensitive_data = bytearray(file_size)

# 2. Read directly into the pre-allocated buffer
with open(key_path, "rb") as f:
    f.readinto(sensitive_data)

# Use the bytes as needed

# 3. Securely overwrite the memory with zeros
for i in range(len(sensitive_data)):
    sensitive_data[i] = 0

# 4. Now safe to remove the reference
del sensitive_data

References:

  1. https://stackoverflow.com/questions/728164/securely-erasing-password-in-memory-python#:~:text=The%20correct%20solution%20is%20to,bytearray%22%20instead%20of%20python%20strings.
  2. https://softwareengineering.stackexchange.com/questions/270113/using-a-bytearray-rather-than-a-string-to-store-password-in-memory#:~:text=Using%20a%20bytearray%20datatype%20to,in%20python%203.4%20using%20bytearray?

Copy link
Contributor

@mkitti mkitti left a comment

Choose a reason for hiding this comment

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

Consider using a bytearray and perhaps a context manager to minimize time that a private key spends in resident memory of the Python process.

Ref: https://softwareengineering.stackexchange.com/a/270159


class SSHKeyContent(BaseModel):
"""SSH key content - only fetched on demand"""
key: str = Field(description="The requested key content")
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider making the private key contents a mutable bytearray that we can explicitly overwrite to clear from memory. Avoid converting the bytearray to an immutable string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could make SSHKeyContent follow the context manager protocol so we can use a with statement. On __exit__ the context manager would then make sure to shred the key contents in memory by zeroing out the resident bytearray. You could use contextlib to help with this.

I also suggest centralizing the the potentially sensitive reading implementation here.

  1. The class would only store a string or Path containing the location of the key file.
  2. The context manager __enter__ would read the file contents into a bytearray via readinto and then return that bytearray
  3. The context manager __exit__ would then explicitly overwrite the bytearray.

This will minimize the time the any sensitive secret spends in resident memory.


class GenerateKeyRequest(BaseModel):
"""Request body for generating an SSH key"""
passphrase: Optional[str] = Field(default=None, description="Optional passphrase to protect the private key")
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider making the passphrase a mutable bytearray that we can explicitly overwrite to clear from memory. Avoid converting the bytearray to an immutable string.

if not os.path.exists(private_key_path):
raise ValueError(f"Private key '{filename}' not found")
with open(private_key_path, 'r') as f:
return SSHKeyContent(key=f.read())
Copy link
Contributor

Choose a reason for hiding this comment

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

Use readinto to read into a mutable bytearray.

with _get_user_context(username):
try:
ssh_dir = sshkeys.get_ssh_directory()
return sshkeys.get_key_content(ssh_dir, "id_ed25519", key_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shred the key contents in a bytearray after usage. Ensure the contents get shreded even if an error occurs. Consider using a context manager here.

@krokicki
Copy link
Member Author

krokicki commented Jan 14, 2026

Consider using a bytearray and perhaps a context manager to minimize time that a private key spends in resident memory of the Python process.

Great ideas, @mkitti. However, this level of security is not priority given that we are running this on our private network and on a server accessible only by a few administrators. I'll add it to our backlog in case anyone has time to revisit it later.


# Append the key
with open(authorized_keys_path, 'a') as f:
f.write(public_key + '\n')
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a restrict keyword here? Does Seqera need an interactive terminal or port forwarding capabilities?

https://manpages.debian.org/experimental/openssh-server/authorized_keys.5.en.html#restrict

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could also restrict what domain names or IP addresses this key can be used from?
https://manpages.debian.org/experimental/openssh-server/authorized_keys.5.en.html#from=_pattern-list_

Copy link
Contributor

Choose a reason for hiding this comment

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

At the end could we add a comment as the last space delimited field that says this was added by fileglancer for seqera?

@mkitti
Copy link
Contributor

mkitti commented Jan 14, 2026

If I understand correctly, the exclusive user of the private key should be Seqera alone. The private key does not need to be used by the user for any other purpose. Why do we need to store the private or public key to disk at all other than adding it to the authorized_keys file?

Rather we only need to transiently show the private key to the user via the web interface to copy into Seqera. If they fail to do so, then we just generate another private key anew the next time.

That process would work more like how Github personal access tokens work. Take a look at how the user interface works for tokens.

image

If you push the "Generate a new token" button and fill out the form, it will show you the token once in this example (I deleted the key before posting the screenshot):

image

It warns that you will not be able to see the personal access token again. Similarly, we can issue the same warning when showing the user the private key for Seqera. If they lose the key, we can just generate another private key.

In summary, there is no need to write the private key to ~/.ssh/id_ed25519 or the public key to ~/.ssh/id_ed25519.pub. Rather we can generate the private key and the public key at some temporary file location or just just store it transiently in memory. All we need to do is add the public key to the ~/.ssh/authorized_keys file, preferably with restrictions and a comment, and show the private key to the user once via the web interface. The private key never needs to be seen again.

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