-
Notifications
You must be signed in to change notification settings - Fork 3
faux-mgs: tech port unlock with permslip #434
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
Conversation
735c4a6
to
c80fb4f
Compare
c80fb4f
to
c691112
Compare
faux-mgs/src/main.rs
Outdated
let mut permslip = Command::new("permslip") | ||
.arg("sign") | ||
.arg(key_name) | ||
.arg("--sshauth") | ||
.arg("--kind=tech-port-unlock-challenge") | ||
.stdin(Stdio::piped()) | ||
.stdout(Stdio::piped()) | ||
.stderr(Stdio::inherit()) | ||
.spawn() | ||
.context( | ||
"unable to execute `permslip`, is it in your PATH and executable?", | ||
)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to others but I have a light preference to also allow overloading the path via environment variable e.g. PERMSLIP
if I need to test something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, added in 68df5ec.
gateway-messages/src/sp_impl.rs
Outdated
|
||
/// Unlocks the tech port if the challenge and response are compatible | ||
fn unlock( | ||
&mut self, | ||
vid: Self::VLanId, | ||
challenge: UnlockChallenge, | ||
response: UnlockResponse, | ||
time_sec: u32, | ||
) -> Result<(), MonorailError>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the advantage of moving this into the trait? Is this to avoid the extra monorail step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not worked in this code base before, so it may just be a mistake. But I was guided by the module-level doc-comment //! Behavior implemented by both real and simulated SPs.
I figured that with oxidecomputer/omicron#8994, the trait would now be the right place for this operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing techport unlock is a monorail command e.g. https://github.com/oxidecomputer/rackletteadm/blob/c27d99ad8558f61bcbf2d7b92dfc1599b393c92d/scripts/common/unlock-techport.sh#L17 and is treated as a component action. My preference would be to keep that same behavior unless there's something speciically preventing us from continuing to use the monorail command. This trait would require changes in hubris. Can you implement a faux monorail command in the simulator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, removed from the trait in 9ddcf1e and pushed a corresponding update to oxidecomputer/omicron#8994.
9cee1dd
to
9ddcf1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/// Use the Online Signing Service with `permslip` | ||
#[clap( | ||
short, | ||
long, | ||
alias = "online", | ||
conflicts_with = "list", | ||
requires = "key" | ||
)] | ||
permslip: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make permslip
conflict with ssh_auth_sock
at the clap level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, added in b79fe36.
In addition to producing signatures over a technician port unlock challenge via
ssh-agent
, also support online signature and response generation viapermslip
(with authn byssh-agent
). See oxidecomputer/permission-slip#252.