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(websocket_client): Add option to set and use cert_common_name in Websocket client (IDFGH-12926) #583

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

DCSBL
Copy link
Contributor

@DCSBL DCSBL commented May 31, 2024

Set a common name that is verified. We use this via a server using a custom certificate bundle + common name combo.

I am not really sure if we need to deny when skip_common_name && cert_common_name is set.

@CLAassistant
Copy link

CLAassistant commented May 31, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title feat(websocket_client): Add option to set and use cert_common_name in Websocket client feat(websocket_client): Add option to set and use cert_common_name in Websocket client (IDFGH-12926) May 31, 2024
@gabsuren gabsuren self-assigned this Jun 25, 2024
@gabsuren
Copy link
Contributor

gabsuren commented Jul 2, 2024

@DCSBL Thank you for your contribution.
However, the changes are causing compilation issues in our CI pipeline. Please refer to the details here:
https://github.com/espressif/esp-protocols/actions/runs/9318507655/job/26646687075?pr=583

@DCSBL
Copy link
Contributor Author

DCSBL commented Jul 2, 2024

Hi @gabsuren, I think I've resolved the issues found by the workflow. Maybe it now detects some other issues but let's see.

I've added a check for the version as setting common name is only available since esp-idf 5.1 (espressif/esp-idf@ce32183). I am not sure if this is the way to go, we could also bumb the minimal required version but that seems a bit aggressive to me.

@gabsuren
Copy link
Contributor

@DCSBL thank you for your contribution. the changes LGTM.
Please fix the commit message format, so we can merge it.
You can install the pre-commit hook as explained in our contributing guide and fix the issues.

@DCSBL DCSBL force-pushed the ws-client-common-name branch from 6d12198 to 077e8d6 Compare July 10, 2024 07:13
@DCSBL DCSBL force-pushed the ws-client-common-name branch from 077e8d6 to 3a6720d Compare July 10, 2024 07:19
@DCSBL
Copy link
Contributor Author

DCSBL commented Jul 10, 2024

👍 all the pre-commit checks pass on my side.

@gabsuren gabsuren merged commit e6f9fe2 into espressif:master Jul 10, 2024
48 of 49 checks passed
@DCSBL DCSBL deleted the ws-client-common-name branch July 10, 2024 08:36
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.

4 participants