Allow importing multiple ssh keys when configuring users#3274
Allow importing multiple ssh keys when configuring users#3274
Conversation
mchf
force-pushed
the
multiple_ssh_keys
branch
2 times, most recently
from
80862f5 to
4b11547
Compare
mchf
force-pushed
the
multiple_ssh_keys
branch
3 times, most recently
from
a03b61a to
df01193
Compare
imobachgs
left a comment
imobachgs
left a comment
There was a problem hiding this comment.
This PR does not implement exactly what we agreed on. We expected to:
- Extend the existing
sshPublicKeyattribute to support a single string or an array of strings. - Add an alias
sshPublicKeys.
| ssh_keys | ||
| .iter() | ||
| .try_for_each(|ssh_key| -> Result<(), service::Error> { | ||
| writeln!(authorized_keys_file, "{}", ssh_key.trim())?; |
There was a problem hiding this comment.
Would not be easier to just "join" all the keys and write them at once?
There was a problem hiding this comment.
well I'd need to trim them anyway. Is there any benefit? I don't expect that many keys that it could get out of fs cache and cause speed issues
There was a problem hiding this comment.
Me neither, but it would be more readable in my opinion. And you only write to disk in one shot. But it is fine as it is.
| assert!(!root_with_ssh_key_config.is_empty()); | ||
|
|
||
| let root_with_ssh_keys = RootUserConfig { | ||
| ssh_public_key: Some(StringOrList::List(vec!["12345678".to_string()])), |
There was a problem hiding this comment.
I would define a StringOrList::from_vec (or something similar). But it is not important, as it is only used in the tests.
There was a problem hiding this comment.
well i tend to not add code only due to tests.
There was a problem hiding this comment.
As you prefer. But if it helps to make the tests easier to understand and maintain...
| @config = result.config | ||
| end | ||
|
|
||
| def basic_user_info(user) |
There was a problem hiding this comment.
To be honest, I do not see the point of moving these two assigments to a separate function. But I can live with that.
There was a problem hiding this comment.
it was because of rubocop ... coplained for complexity.
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub ssh_public_key: Option<String>, | ||
| #[serde(alias = "ssh_public_keys")] | ||
| #[schema(inline)] |
There was a problem hiding this comment.
Why do you need this schema(inline)?
There was a problem hiding this comment.
OpenAPI otherwise complains on StringOnList ... another option was do a nontrivial update in web server, the components method.
There was a problem hiding this comment.
But does it generate the proper OpenAPI specification? Just asking.
There was a problem hiding this comment.
Yes, it does, you can check it by running the command cargo xtask openapi.
{
"type": "object",
"properties": {
"sshPublicKey": {
"oneOf": [
{
"type": "null"
},
{
"oneOf": [
{
"type": "string"
},
{
"type": "array",
"items": {
"type": "string"
}
}
]
}
],
"description": "Root SSH public key"
}
}
}
mchf
force-pushed
the
multiple_ssh_keys
branch
2 times, most recently
from
7275c11 to
830664a
Compare
| .as_ref() | ||
| .map(|k| k.to_vec()) | ||
| .unwrap_or_default(); | ||
| // do some magic about user's home dir path or stay |
There was a problem hiding this comment.
I would say it is fine. Perhaps we could move the string to a const, but that's all.
2 participants
Problem
Currently we allow to import only one ssh key for root user. We are asked to be able to 1) import multiple keys for root 2) as well for common user
Solution