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: client certificate support #275

Merged
merged 1 commit into from
Oct 15, 2023

Conversation

phoval
Copy link
Contributor

@phoval phoval commented Oct 1, 2023

Add initial mutual TLS support with client certificate in global preferences

@phoval phoval force-pushed the feat/mTlsInitialSupport branch 5 times, most recently from 59e5a42 to fe773ca Compare October 8, 2023 10:05
@helloanoop
Copy link
Contributor

Apologies, I haven't gotten to this PR since last week. This will be a priority item on my list to get it merged this week.

@adiesner
Copy link

adiesner commented Oct 9, 2023

@phoval thank you for taking care. I do miss that feature in Bruno too.

But looking at your implementation it looks like you only support one client certificate and one domain. I think it is very common that you need to configure multiple client certificate for multiple domains. Just thinking about a preview and a prod environment.

Do you have time to extend your implementation?

@phoval phoval force-pushed the feat/mTlsInitialSupport branch 3 times, most recently from 6faa6ba to 0da1769 Compare October 10, 2023 16:45
@phoval phoval changed the title feat: initial mTLS support feat: client certificate support Oct 10, 2023
@phoval
Copy link
Contributor Author

phoval commented Oct 10, 2023

Just moved the config in the collections settings. Made possible multiple client certificate.
UI still need to be polish.
Maybe encode the certificate/key file in base64 for easier git sync?

@phoval phoval force-pushed the feat/mTlsInitialSupport branch from 1ba86c1 to 18e0239 Compare October 11, 2023 05:52
@panda7789
Copy link
Contributor

@phoval 👍 for your work.

Just a little error I found,
config must contain at least empty collection of clientCert:
image
When collection is not present, it fails on .push into undefined:
image

I think you didnt notice, because you already have it in config.

@max-wittig
Copy link
Contributor

Thanks for all the work here! This is really the last blocker to switch away from the buggy, data hungry Insomnia

@mjhcorporate
Copy link
Contributor

mjhcorporate commented Oct 12, 2023

Thank you! We are also waiting on this feature to be able to switch away from the commercial tools.

For reference: This is what the UI to set the certificate looks like (I like it!)
image

But: When I click "add", the values are not persisted. When I navigate away from the UI, save another request, and then come back, the UI is reset to all empty values.

@panda7789
Copy link
Contributor

@mjhcorporate Try to add into config this empty collection
image
And than try save cert again. After click on add it should be listed on upper list, where you now have "None".

@mjhcorporate
Copy link
Contributor

mjhcorporate commented Oct 12, 2023

Ahh, that was the error you mentioned earlier. Thank you, now it works!

But: Saving the key in the config has two downsides for us:

  • (minor): The certificates expire and need to be refetched daily. I have a script that updates them on my harddrive. How would I trigger a re-import?
  • (major): I am tracking my bruno.json in git. Under no circumstances must I upload the secret key to GitHub!

Proposal: Rename cert and key to certPath and keyPath, and store the path to the files. Then, on each request, read the file contents (no caching please). That way, the certs are guaranteed to be fresh. We lose a few ms for the file system access, but I think it's worth it in this case. This is also the path that Postman and Insomnia took.

@phoval phoval force-pushed the feat/mTlsInitialSupport branch from 28d9206 to e589184 Compare October 12, 2023 17:52
@helloanoop
Copy link
Contributor

I am tracking my bruno.json in git. Under no circumstances must I upload the secret key to GitHub!

@mjhcorporate Checkout two options for managing secrets without having to commit credentials to git - https://docs.usebruno.com/secrets-management/overview.html

To interpolate env vars, you need to change

httpsAgentRequestFields['passphrase'] = clientCert.passphrase;

to

httpsAgentRequestFields['passphrase'] = interpolateString(clientCert.passphrase, interpolationOptions)

You can checkout how we do this in proxy

@phoval Can you share how the json schema will look like in bruno.json
I recommend to use the full name instead of shorthand.

{
  "clientCertificates": [{
    "certFilePath": "assets/cert.pem",
    "keyFilePath": "assets/cert.key",
    "passphrase": "{{process.env.clientCertPassPhrase}}"
  }]
}

But: When I click "add", the values are not persisted. When I navigate away from the UI, save another request, and then come back, the UI is reset to all empty values.

You need to implement the save functionality. Checkout how it's done in Proxy Settings Tab

Glad to see the progress !! @phoval @mjhcorporate Let me know as soon as you guys are in agreement. Then we can merge.

Copy link
Contributor

@mjhcorporate mjhcorporate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I retested, and it looks great! Thank you so much for your amazing work, @phoval !

Minor nitpicks:

  1. There is no user-facing error when the filepath becomes invalid (file is deleted or moved). Ideally, the user would see an error message, just like the one they see when the cert-file is corrupted.
  2. Absolute filepaths and relative filepaths work, but ~ does not.
  3. .env-variables are not permitted for the paths. When entering "certPath": "{{process.env.clientCertPath}}", I get Error reading cert/key file Error: ENOENT: no such file or directory, open '{{process.env.clientCertPath}}'
    • If this would be possible, then the nitpick above is solved, as every user can expand their ~ themselves, manually
  4. I happen to have my cert in a directory that starts with a ., and to show that directory in the MacOS filepicker, I have to press CMD + Shift + ., every time I open the picker. It would be amazing if the UI remembered my show-dot-files choice.
  5. The UI does not have enough space to show the chosen filenames or the "nothing selected" strings (see "Kein...ählt" and "car-...key" in the screenshot below)
    image
    @helloanoop Everything good from my side! In my view, these points are minor and could be addressed by follow-up PRs some time in the future.

@max-wittig
Copy link
Contributor

@mjhcorporate Exactly. Let's get this in so people can use it and then the nitpicks can be addressed later.

@phoval phoval force-pushed the feat/mTlsInitialSupport branch from 7580992 to 8ee9c0e Compare October 13, 2023 20:24
@phoval phoval force-pushed the feat/mTlsInitialSupport branch from 8ee9c0e to d6628d9 Compare October 13, 2023 20:30
@phoval
Copy link
Contributor Author

phoval commented Oct 13, 2023

@helloanoop changed the configuration name to the longer one and fixed the interpolation options.

"clientCertificates": [
    {
      "domain": "*.example.org",
      "certFilePath": "client.pem",
      "keyFilePath": "client.key",
      "passphrase": "test"
    }
  ]

It should be ready for merging now.

@helloanoop helloanoop merged commit faa7cdb into usebruno:main Oct 15, 2023
1 check passed
@helloanoop
Copy link
Contributor

Thanks @phoval for adding Client Certificates support in Bruno
I appreciate your patience and efforts towards incorporating the feedback from the community.

I added some polish 333564f
As well as updated the json structure to allow for enabling/disabling client certificate usage.

  "clientCertificates": {
    "enabled": true,
    "certs": [
      {
        "domain": "localhost",
        "certFilePath": "/Users/anoop/bruno/bruno-testbench/certs/server.crt",
        "keyFilePath": "/Users/anoop/bruno/bruno-testbench/certs/server.key",
        "passphrase": ""
      }
    ]
  }

image

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.

6 participants