CUT-5090: Secure credential reference in settings#744
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
jworkmanjc
left a comment
jworkmanjc
left a comment
There was a problem hiding this comment.
Okay this is very close to what we are looking for here. In general yes, this is close to the Acceptance Criteria in the card.
Connect-JCOnline -Select
If no key in Credential Manager or Keychain ends with "api.jc" then we prompt for a key, ask the customer to name it. That's mostly the functionality in the body of work. I love it, it's easy to use needs a bit of work to clean things up but in general works and is easy enough for someone to use.
Really this is solving a big issue for customers, they need to copy & paste their API key in every time the want to use the module. Now they can just select a saved one they have in encrypted storage. Great change.
One issue I saw that needs specific addressing. You can't copy data into the prompt when asked to "Type the api key:" I had to manually try to type my key and I mis-typed it the first time. I had to go edit it in credential manager but did get it to connect.
MacOS Keychain worked for me.
A few other things I think we'd need to do before marking this in the next state for review:
- Removing items from Keychain/CredManager needs to be behind a prompt. I accidentally removed my key in Keychain and certainly didn't mean to.
- We probably need to submit for security review this change, it will be an overall improvement on security posture I believe.
- We'll want to merge this into a release branch, not
masterbranch, we can merge when we get the go-ahead with security.
| Write-Host "Select the JumpCloud Api Key. Press [Escape] to type a new key. Press [Backspace] to remove the selected key" -ForegroundColor Green | ||
| while (@($false, $null) -contains ($vaultKey = Find-Interactive -choices $keys -Callback { | ||
| param($param) | ||
| Remove-FromVault -Key $param; |
There was a problem hiding this comment.
I did accidentally call this function and want to be super careful about how we think about implementing this. IMO there should be a confirmation before deleted any Key.
I wasn't super clear on usage first time so I created a key by accident titled "api.jc.api.jc". Somehow it got deleted when I ran Connect-JCOnline -select "api.jc.api.jc
There was a problem hiding this comment.
Added confirm dialog with "Yes"/"No"
| )] | ||
| [Switch]$force | ||
| [Switch]$force, | ||
| [Parameter( |
There was a problem hiding this comment.
Okay awesome to see this here, I kind of knew this was going to be a switch without looking at the code based on the PR. Totally fine, thinking about automation scenarios, it would be good for us to include a way to specify the stored key.
We can create a separate card for this but customers should be able to type:
Connect-JCOnline -Select -Credential "myKey.api.jc" and connect to that org
There was a problem hiding this comment.
Added -Credential param Connect-JCOnline -Credential "ok.api.jc".
It will jump the -select param for listing the keys.
| [Parameter(Mandatory=$true)] | ||
| [string]$Key, | ||
| [Parameter(Mandatory=$false)] | ||
| [string]$sufix, |
There was a problem hiding this comment.
Curious about the choice of suffix here over prefix. For my reference, keys are gathered and parsed to see if they end with some suffix.
I think that results in better looking keys as you are selecting them like:
- OrgName.api.jc
- AnotherOrgName.api.jc
- LastOrg.serviceTotken.jc (assuming we add support for service token credentialing later)
Reference Code:
https://github.com/TheJumpCloud/support/pull/744/changes#diff-2283fb1a2772f5d917313a140539f71ffb3860db63e32f4d1c0b4e1fec3549a1R11
https://github.com/TheJumpCloud/support/pull/744/changes#diff-2283fb1a2772f5d917313a140539f71ffb3860db63e32f4d1c0b4e1fec3549a1R20
lucasmendes-jc
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Reviewed by Cursor Bugbot for commit e5c5a99. Configure here.
| cred.targetName = target; | ||
| cred.comment = null; | ||
| cred.lastWritten = 0; | ||
| cred.credentialBlobSize = (secret.Length + 1) * 2; |
There was a problem hiding this comment.
Credential blob size includes null terminator causing authentication failures
High Severity
SetCreds sets credentialBlobSize to (secret.Length + 1) * 2, which includes the null terminator in the stored byte count. When GetCreds later reads the credential using Marshal.PtrToStringUni(cred.credentialBlob, cred.credentialBlobSize / 2), the length-based overload copies exactly that many characters — including the null terminator as a real character in the C# string. The returned API key will have a trailing \0 appended, causing it to differ from the original secret and likely causing authentication failures.
Additional Locations (1)
- [
PowerShell/JumpCloud Module/Private/Vault/Unlock-Platform.ps1#L64-L65](https://github.com/TheJumpCloud/support/blob/e5c5a99d1f8b68b2808129cf4f140eea2ddfcc63/PowerShell/JumpCloud Module/Private/Vault/Unlock-Platform.ps1#L64-L65)
Reviewed by Cursor Bugbot for commit e5c5a99. Configure here.
2 participants
Issues
CUT-5090
Avoid user to directly prompt key on console.
Hide key native console functions.
No external module.
What does this solve?
Gets from the credential manager when asked from module/prompt.
By default needs the
-selectparam or$null -eq $env:JCApikKeyWhen asked from prompt it should:
.api.jcby defaultIs there anything particularly tricky?
Unlock-Platform is used to compile C# code on Win platform.
How should this be tested?
Connect-JCOnline.ps1 -selectAlso can run the :
/PowerShell/JumpCloud Module/Tests/Public/Authentication/Connect-JCOnline.Tests.ps1Screenshots
Note
High Risk
Changes authentication and how API secrets are stored and read (Keychain, Windows CredWrite with LOCAL_MACHINE persistence, and dynamicparam key resolution), which are security-sensitive and affect every Connect-JCOnline session.
Overview
Connect-JCOnlinenow resolves the API key from the OS credential store instead of typing it on the console whenever-Selectis used,-Credentialnames a vault entry, or$env:JCApiKeyis unset (unless-JumpCloudApiKeyis passed on the command line). New-Selectand-Credentialparameters drive an interactiveKeySelectorflow that lists keys ending in.api.jc, prompts to add keys via secure input, removes entries on Backspace (with confirm), and adds a new key on Escape.Supporting Private helpers add a small console UI (
Find-Interactive,Confirm-Console,Clear-Console) and vault operations (Unlock-Platform,Get/Set/Removevault,Get-VaultKeys) backed by macOS Keychain (security) and Windows Credential Manager (inlineCredManagerC# viaadvapi32). Linux vault use is explicitly unsupported in these paths.Reviewed by Cursor Bugbot for commit e5c5a99. Bugbot is set up for automated code reviews on this repo. Configure here.