Skip to content
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

Feat: Add support for PKCS12 / PFX client certificates for mTLS #2336

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pietrygamat
Copy link
Contributor

@pietrygamat pietrygamat commented May 18, 2024

Description

This change allows bruno to use client certificates stored in PFX / PKCS12 files in addition to regular PEM cert/key pairs.
Updated the Client Certificate selection in Collection view so that the user can specify if they are using pfx format, in which case the key file becomes unnecessary, and the control disabled. The passphrase field works for both formats.

Axios used by bruno has the support built-in so this change is a rather trivial one, with only the question of UI to solve.

resolves #1889
resolves #1698

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Screenshot from 2024-05-18 02-05-06
Screenshot from 2024-05-18 02-04-52

For reference: Postman implementation
Screenshot from 2024-05-18 02-16-20

@pietrygamat pietrygamat force-pushed the feature/client-certificate-p12 branch from 6312a86 to 07d3089 Compare May 20, 2024 19:39
@helloanoop
Copy link
Contributor

Thank you @pietrygamat !

@lohxt1 Lets target this to have it merged for next weeks release.
Assigning it to you.

@thalesog
Copy link

thalesog commented Jun 10, 2024

Guys, can we get this PR merged, please?

@jimbase
Copy link

jimbase commented Jun 10, 2024

+1 :-) I want to present bruno to my colleagues as the better alternative to postman - but I need the change bc of our client certificates environments

@nddipiazza
Copy link

When clicking on the Add certificate button, it says "Successfully added client certificate" but nothing appears in the Client Certificates list

@nddipiazza
Copy link

ignore my last comment.
when updating Bruno on windows using the Exe... the exe does not seem to update gracefully
we had to delete %LocalAppData%\Programs\bruno then run the exe to update. then it worked.

@nddipiazza
Copy link

adding a +1 from community here. this fixed our problem.

@helloanoop
Copy link
Contributor

Hey @pietrygamat There are some issues with this PR. The pfx flag is not persisted between app launches in bruno.json

"clientCertificates": {
    "enabled": true,
    "certs": [
      {
        "domain": "www.usebruno.com",
        "certFilePath": "/Users/anoop/foo.cert",
        "keyFilePath": "/Users/anoop/bar.key",
        "passphrase": ""
      }
    ]
  },

I recommend saving an additional key called pfxFilePath - and the logic being that

  • we use the pfxFilePath if it is present
  • else we use the certFilePath and keyFilePath

What do you think?

@pietrygamat
Copy link
Contributor Author

pietrygamat commented Jun 14, 2024

Hey @pietrygamat There are some issues with this PR. The pfx flag is not persisted between app launches in bruno.json

"clientCertificates": {
    "enabled": true,
    "certs": [
      {
        "domain": "www.usebruno.com",
        "certFilePath": "/Users/anoop/foo.cert",
        "keyFilePath": "/Users/anoop/bar.key",
        "passphrase": ""
      }
    ]
  },

Hmm... I cannot reproduce this... Once I click add, bruno.json is updated immediately with 5 fields, and the entry is added to the Client certificates list. Do you mean the UI state here? I guess it should be reset after clicking Add to confirm the operation is done, but that's that.

I recommend saving an additional key called pfxFilePath - and the logic being that

  • we use the pfxFilePath if it is present
  • else we use the certFilePath and keyFilePath

What do you think?

Let me clarify:
Are you proposing to change the json format, but keep the UI as proposed? That toggling the pfx checkbox would result in moving the file path between pfxFilePath and certFilePath of underlying object?

Or are you proposing to go Postman route and present the user with 3 input fields instead of 2, and prioritize the pfx when prepping request, but keep all 3 values in json? My problem with that is the logic to prioritize one over the other must be communicated to the user somehow. Postman sucks for just that - not communicating what happens here: do I have to fill all the fields? If I do, which will postman choose? Maybe both? But where does the passphrase apply then? To pfx or to encrypted key?
Screenshot from 2024-06-15 00-46-04

Insomnia does a better job at explaining that these are either-or values, but still accepts all inputs at once, ending up with confusing state of having pfx, key and passphrase (for key? pfx?) in the UI:
Screenshot from 2024-06-15 00-53-50 Screenshot from 2024-06-15 00-59-08

To avoid that, and do better, we should make the UI disable incompatible inputs depending on user first selection, but in a more complex scenario like this there is much more logic to add. For example something as easy as unselecting a file from input before submitting the form - we are missing a clear/reset button now, so to unload a file you have to open file chooser, select nothing and click cancel* . That's not ideal flow, but so far changing it has been out of scope of this PR. It will no longer be, if we aim to not be sloppy :).

*) Note, this crashes current release, btw, fixed in this PR

…ar form on submit, clear file inputs to visually match underlying object
@pietrygamat pietrygamat force-pushed the feature/client-certificate-p12 branch from bf4eb21 to 1d64636 Compare June 15, 2024 00:10
@pietrygamat
Copy link
Contributor Author

I guess it should be reset after clicking Add to confirm the operation is done, but that's that.

I added additional change to cover that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importing PFX in Client Certificates should not require Key File Question: Using Keystore .p12
6 participants