bruno icon indicating copy to clipboard operation
bruno copied to clipboard

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

Open pietrygamat opened this issue 1 year ago • 1 comments

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:

  • [x] The pull request only addresses one issue or adds one feature.
  • [x] The pull request does not introduce any breaking changes
  • [x] I have added screenshots or gifs to help explain the change if applicable.
  • [x] I have read the contribution guidelines.
  • [x] 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 avatar May 18 '24 00:05 pietrygamat

Thank you @pietrygamat !

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

helloanoop avatar May 22 '24 13:05 helloanoop

Guys, can we get this PR merged, please?

thalesog avatar Jun 10 '24 13:06 thalesog

+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

jimbase avatar Jun 10 '24 14:06 jimbase

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

nddipiazza avatar Jun 12 '24 19:06 nddipiazza

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 avatar Jun 12 '24 20:06 nddipiazza

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

nddipiazza avatar Jun 13 '24 00:06 nddipiazza

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?

helloanoop avatar Jun 14 '24 11:06 helloanoop

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

pietrygamat avatar Jun 14 '24 23:06 pietrygamat

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.

pietrygamat avatar Jun 15 '24 00:06 pietrygamat

This has been taken care in https://github.com/usebruno/bruno/pull/2482

Thanks for the work on this @pietrygamat for the initial work on this feature! I deeply appreciate you helping us out in the review and approving the PR https://github.com/usebruno/bruno/pull/2482

helloanoop avatar Jun 21 '24 05:06 helloanoop