This repository was archived by the owner on Jan 22, 2025. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New Feature: Two Man Realms #68
base: master
Are you sure you want to change the base?
New Feature: Two Man Realms #68
Changes from 15 commits
76a1f7e
8e3a92e
1c64f9b
78e9fe2
558f443
0e0648a
2924e62
e132a11
8c5bd38
1c35692
c386689
7c06967
5d915e6
c9724fa
14bfca8
11d3e07
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
TWO_MAN_
is a little confusing since the number of approvers is variable. it sounds to me like this is an M of N control policy. can we call it something likeis it possible we could want multiple control policies managed by a single bot? if so, this might need to be more complex. that's why i shoved a 1 in there. maybe not necessary right now. i dunno.
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.
Done! I think for now it isn't necessary to have the 1 in there since that seems like a pretty advanced feature and probably beyond the scope of this project.
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.
this is kind of a mouthful. any chance it can work more like:
OR
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.
Sadly, there isn't really a way to make that work :/
In your first example, there is no way for kssh to know which servers need which principals. This becomes especially hard when talking about ssh configs which can define custom names like "production" for a hostname.
The second example runs into essentially the same issue. While kssh could attempt to go first and then alert the user if the connection fails, there is no way for
kssh --request root@production
to know which principals to request.A feature that could be built out is allowing the user to configure this sort of thing on the client side. Eg:
The difficulty with the local config strategy is that it will break in weird ways when using proxy commands or any more advanced SSH features. Eg
kssh -J root@production david@other
. kssh wouldn't see theroot@production
connection and wouldn't know to provision a certificate with that principal. So IMO it doesn't seem worth it to build this feature given how brittle it would end up being, but certainly open to it if you think it would still be useful.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.
it seems really sub-optimal to me to push the onus of knowing how everything is set-up down to the kssh user.
but doesn't the bot know? or if not, can we make it know with additional environment variables?
it looks like the message requesting approval is coming from the kssh user. can it come from the bot instead? this will make it much easier for an operations team to iterate on how they want this thing to work for them without having to push an update to all kssh users, which i imagine would be much more difficult.