-
Notifications
You must be signed in to change notification settings - Fork 3
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
The pk secrets env
command for meeting Development Environment Usecase
#31
Comments
Note that the |
So would the |
No it would have to stay as a C++ file, cause it's native functionality.
It's mostly about updating the C++ code so that it is compatible with
our current node version.
…On 7/14/21 1:05 PM, scottmmorris wrote:
So would the |kexec.cc| file need to be rewritten in JS for it to be
usable inside js-polykey?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/MatrixAI/js-polykey/issues/205#issuecomment-879549093>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4OHKVPQI6FCUD733G5KTTXT5JBANCNFSM5ACCXO5A>.
|
At this point, the main points of review are:
Testing of the env subcommand requires a separate process context because eventually it should be using a So we will need to use a variant of The other challenge with testing this, is that if you are testing a subshell like However if you write a subprogram like this:
During the testing:
The The above script uses There are also platform requirements. For example on Windows, you'll need to use a powershell script. Let's put the platform specific testing into a separate issue later. |
Put on hold until we have client-refactoring merged. |
I found my old comment from last year talking about this: MatrixAI/Polykey#55 (comment). It also has a "chording" idea that may be relevant. |
Example of this being done for Hashicorp Vault: https://github.com/channable/vaultenv. Note that process replacement doesn't exist on windows. It will always be using a child process. |
Wanted to mention that this command will be essential to obsolete the usage of The usage of Screen sharing can occur during meetings, recorded work sessions, or when requesting external help. The We've seen this happen a few times, and I think there was a major leak that occurred with this. |
MatrixAI/Polykey#505 was closed because this now has to be implemented in Polykey-CLI. However it can be reviewed for notes and design ideas. |
New update https://github.com/StorytellerCZ/node-kexec. I think we should be able to integrate this into our own library We can incorporate windows support with this understanding:
|
On request I'm adding this issue to the #40 epic |
Some notes about the scope.
So there are a few components to this that need to be addressed and specced out.
|
I'll start with scaffolding the command. |
I think put a simple C++ or rust code into it to expose the proper exec. For windows it's not possible so we won't bother. The rust is likely far simpler than the C++ scaffold with the bonus of being similar to js-quic and subsequently deno. |
Note that @amydevs has experience with the setup of a partial native module. |
Will we need to do any native code if we use https://github.com/StorytellerCZ/node-kexec ? |
As for specifying the enviroment variable we want with the command. Commander provides a varadic option. https://github.com/tj/commander.js?tab=readme-ov-file#variadic-option This will allow us some freedom in specifying the enviroment variables that we will get as an array that can be parsed. |
I don't want to use other people's native code. It will be flexible. This is a simple enough function for our own js-exec. |
I think cpp would be better to use than Rust here. If we were to use Rust, we would need to bring in native bindings to apis and writing unsafe code anyways. Also memory safety shouldn't be hard with the limited complexity of it anyways. |
I'd lean towards not deviating from how we do native bindings in other repos. If only to reduce entropy by having everything stick to a standard way of handling it. |
We should be migrating to rust - for demo compatiblity. Plus the js-quic is much more modern package distribution compared to js-db. Everything new native should be rust. |
@tegefaulkes is there anything in the original issue spec that you're changing? As per this comment #31 (comment) |
It's mostly the same except for the following.
|
Is there a need for PR for PK too? Link them here too. |
It's important for us to be able to source the secrets too so we can use it inside the shell hook @brynblack. |
The |
I don't think that's necessary yet, simply referring to the file path is enough.
Yea that's just Without the command, it can just print out a consumable shell settings. And I believe That will set it to the current shell, but not necessarily export them. To do that:
We do this already in our shellhook. Note that on Windows, the syntax is not compatible. So we would have to have a switch when outputting to powershell format: https://stackoverflow.com/a/51247258.
That's a bit more complicated because the |
Someone came up with a PS script that can interpret unix style syntax: https://stackoverflow.com/a/74839464. That's another way, which is to simply provide that execution and embed the above content like a heredoc. But still doesn't solve the ability to auto-export. It seems weird to have to always set the |
Specification
pk secrets env
command should operate similar to the unixenv
commandpk secrets env
to do double duty: inject environment variables into a new subcommand, or allow sourcing of environment variables into an existing subshellpk secrets env someprogram arg1 arg2
should do a proper process replacement when possibleAdditional context
Copied excerpt:
Tasks
pk secrets env
can be used to inject environment variables and run a subprocesspk secrets env
can be used to source environment variables and output something that can be used as a file to be sourced by a shellThe text was updated successfully, but these errors were encountered: