Skip to content

SSL Support: Hide all HTTPS help messages when the certificate is trusted #1117

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

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Mar 24, 2025

Related issues

Proposed Changes

  • Implements a function to check whether the CA certificate is trusted.
  • hide all the SSL help texts when the CA certificated is trusted.

Testing Instructions

  • Create an SSL site
  • If your CA certificate is not trusted, you should see help text on the creation form and on the settings tab
  • If your CA certificate is trusted, you shouldn't not see either of these.

@youknowriad youknowriad requested a review from a team March 24, 2025 11:29
@youknowriad youknowriad self-assigned this Mar 24, 2025
@youknowriad
Copy link
Contributor Author

I didn't not a add a dialog because the existing help text in the form is enough IMO.

Copy link
Contributor

@bgrgicak bgrgicak left a comment

Choose a reason for hiding this comment

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

It works, but the component didn't update until I switched to another tab and back.
It's not a big deal, but it would be nice if we would periodically check in the background, or add a button so the user can check manually.

Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

I tested it on macOS, and it works almost as expected. Great work!
It would be great, as Bero mentioned, to "refresh" the state when going back to Studio. You can use useWindowListener('focus', yourFunction); to trigger a call when the user focuses back to Studio.
As Wojtek said, "trusted/enabled" with the current implementation means "installed" in the keychain but not necessarily trusted by the user.

I was able to install and trust a certificate and after changing tabs I confirm the message was gone. 👍 .

New site

No CA CA Installed
add-site-show add-site-hide

Edit site

No CA CA Installed
edit-site-show edit-site-hide

While testing this PR I noticed some edge cases that are worth mentioning in case they are not already tracked in issues:

  1. When editing a site > unchecking the HTTPS and saving does nothing.
  2. While saving the edit site modal, every checkbox and input is "disabled" except "Enable HTTPS". Allowing the user to change the checkbox state when the saving is in process.
  3. The documentation (p2gHKz-wsF-p2) mentions I should click on Open the Help → Trust Certificate menu option in WordPress Studio in order to trust the certificate. But actually that button was in Site settings.
  4. When moving from custom domain to default domain, the links in my site point to the old URL, breaking the styles.

Trust certificate

studio-accept-certificate.mp4

Uncheck Enable HTTPS

enable-https-checkbox.mp4

Comment on lines 532 to 533
{ __( 'Learn how' ) }
<span aria-label={ __( '(opens in a web browser)' ) }>&#8599;</span>
Copy link
Member

@sejas sejas Mar 25, 2025

Choose a reason for hiding this comment

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

There should be a space between Learn how and the arrow. Same would apply to the Learn how in edit site overview. I've observed that we are not using the arrow to express external link in other "learn more" links, in my opinion the arrow make sense though.

Screenshot 2025-03-25 at 11 38 58

Figma design ( RToz6tIuQ7nlZrikBte4GU-fi-6924_4203 ):
Screenshot 2025-03-25 at 11 44 47

const platform = process.platform;
if ( platform === 'win32' ) {
try {
const { stdout } = await execFilePromise( 'certutil', [ '-store', 'ROOT', CA_NAME ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to @wojtekn's comment about the security command on macOS, I would use something like certutil -verify ${ CERT_PATH } here and look for the following output (or parts of it):

Verified Issuance Policies: All
Verified Application Policies:
    1.3.6.1.5.5.7.3.1 Server Authentication
    1.3.6.1.5.5.7.3.2 Client Authentication
Cert is a CA certificate
Cannot check leaf certificate revocation status
CertUtil: -verify command completed successfully.

When the certificate is on the keychain but not fully trusted, I get the following output:

Verified Issuance Policies: All
Verified Application Policies: None
Cert is a CA certificate
Cannot check leaf certificate revocation status
CertUtil: -verify command completed successfully.

And when it's not on the keychain, I get the following output:

Verifies against UNTRUSTED root
Cert is a CA certificate
Cannot check leaf certificate revocation status
CertUtil: -verify command completed successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to need your help here. I did make changes for windows, but I'm not sure relying on the output string is good enough (apparently the output might differ), I'm currently relying on the exit code only but I'm also not sure whether that's sufficient, I'd appreciate some testing :)

Copy link
Contributor

@fredrikekelund fredrikekelund Mar 26, 2025

Choose a reason for hiding this comment

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

apparently the output might differ

I understand if you have a general concern regarding the output structure, but I'm still curious if you verified this assumption in testing or in documentation?

exit code

The exit code for certutil -verify doesn't seem to change depending on the certificate trust status in my testing.

I looked closely at the PowerShell PKI module, which, in some ways, seems like a more scripting-friendly option. However, I was not able to get the correct output when the certificate is on the keychain, but not properly trusted (open the certmgr program > Trusted Root Certification Authorities > Certificates > WordPress Studio CA > Properties > Purposes > Disable all purposes). This might seem like an edge case, but I don't have full confidence in the PowerShell approach since it doesn't report this correctly.

Screenshot 2025-03-26 at 10 44 11

Copy link
Contributor

@fredrikekelund fredrikekelund Mar 26, 2025

Choose a reason for hiding this comment

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

I would love to suggest that we employ a different strategy: request a local HTTPS URL and check for HTTPS errors. This would work cross-platform and seems more bulletproof, but it also requires the proxy server to be started (we recently delayed proxy server upstart in #1089).

I'm not sure this is better (or that it would work, for that matter), but I wanted to raise it nonetheless. Have you considered this option, @youknowriad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't not consider it, I'm happy to try it but it would mean reverting the lazy start for the proxy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. The Microsoft docs for certutil do say this:

Certutil isn't recommended to be used in any production code and doesn't provide any guarantees of live site support or application compatibilities. It's a tool utilized by developers and IT administrators to view certificate content information on devices.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our use of certutil wouldn't be mission-critical, but there's clearly an inherent fragility.

Feel free to explore the PowerShell PKI module option to see if that works. Parsing the output should definitely be easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would use an output to avoid additional complexity. If we checked for UNTRUSTED and got an unexpected output, in the worst case scenario users would not see a message asking them to trust the certificate.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we stick with certutil, we should parse the Verified Application Policies: part of the output, too. I'm not sure if both the Server Authentication and Client Authentication policies are needed, but at least one of them is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I've updated the code here to check for the Verified Application Policies but my confidence with my updates is not 100%. I was not able to test despite spending a lot of time trying to setup a windows environment.

That said, even if it fails, the risks are small to the users. (hide/show a message)

@youknowriad youknowriad force-pushed the hide/ssl-help-trusted-certificate branch from aea13b8 to 29a44b1 Compare March 25, 2025 16:56
Comment on lines +186 to +187
'-s',
CA_NAME,
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the manual (man security), option -s is deprecated in favour of -n:

            -s sslHost      Specify SSL host name for the ssl policy. (This option is deprecated;
                            use -n instead.)

            -n name         Specify a name to be verified, e.g. the SSL host name for the ssl
                            policy, or RFC822 email address for the smime policy. For backward
                            compatibility, if the -n option is provided without an argument, it will
                            be interpreted as equivalent to -N.

Copy link
Member

Choose a reason for hiding this comment

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

That said, CA_NAME isn't a host name, it's the string used as certicates' commonName, so I'm not sure what this option adds to the command.

Copy link
Member

Choose a reason for hiding this comment

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

The -n flag is apt for the domain-specific certificates:

security verify-cert -c domains/foo.wp.local.crt -p ssl -s foo.wp.local # ok
security verify-cert -c domains/foo.wp.local.crt -p ssl -s example.org  # nope

but it doesn't work with the CA:

security verify-cert -c studio-ca.crt -p ssl -s example.org # no errors
                                                            # but this is misleading!

Comment on lines +13 to +21
const checkCertificateTrust = useCallback( () => {
getIpcApi()
.isCATrusted()
.then( ( trusted ) => {
if ( isMounted.current ) {
setIsTrusted( trusted );
}
} );
}, [ setIsTrusted ] );
Copy link
Member

Choose a reason for hiding this comment

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

Not super important, but there are a bunch of different identifiers coexisting:

  • use certificate trust
  • check certificate trust
  • is CA trusted
  • is Root CA trusted

With just a little refactoring we could probably just have useIsRootCATrusted and isRootCATrusted. Especially if in ipc-handlers.ts we imported the whole module to avoid collisions:

export async function isRootCATrusted( _event: IpcMainInvokeEvent ): Promise< boolean > {
	return certificateManager.isRootCATrusted();
}

Copy link
Member

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Can't vouch for the Windows side, but LGTM on macOS!

@youknowriad youknowriad requested a review from Copilot March 31, 2025 13:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for hiding HTTPS help messages when the CA certificate is trusted. The changes include introducing a new IPC method and associated hook to check certificate trust, updating site settings logic to conditionally hide SSL help texts, and adding corresponding tests for the new functionality.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/preload.ts Added new ipcRenderer API “isCATrusted” to check certificate trust status.
src/modules/site-settings/edit-site-details.tsx Updated logic to conditionally enable HTTPS based on certificate trust.
src/lib/certificate-manager.ts Implemented isRootCATrusted to verify if the system trusts the CA certificate.
src/ipc-handlers.ts Exposed isCATrusted IPC handler wrapping the underlying certificate trust check.
src/hooks/use-certificate-trust.ts Introduced a React hook to track certificate trust state.
Various test files Added mocks for isCATrusted to simulate trusted certificate scenarios.

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.

Feature Request: Handle already trusted Studio certificate.
6 participants