Add client certificate auth page #8105

Ordinant · 2021-05-27T13:35:17Z

No description provided.

Ordinant · 2021-05-27T13:36:40Z

...

rosewms · 2021-05-27T19:03:16Z

docs/src/main/sphinx/security/cert-auth.rst

+certificates for each client. Sites in this category have an existing PKI
+infrastructure, possibly including an onsite CA.
+
+This feature is *not* appropriate for sites that would need to generate a set of


I don't think we need to emphasize "not"

Again, we're trying to steer users away from this page. Only 1 in 500 sites will need it.

In other places we use strongly suggest etc. With bold. I suggest we stick with the bold convention for any emphasis.

I am in favour of leaving as is. "Strongly suggest" just feels too weak..

docs/src/main/sphinx/security/cert-auth.rst

rosewms · 2021-05-27T19:12:28Z

docs/src/main/sphinx/security/cert-auth.rst

+
+Trino can be configured to support certified clients seeking validation from a
+Trino server. This authentication method is only provided to support sites that
+have an absolute requirement for client authentication *and already have* client


Is there guidance in the Google Dev TW guide about emphasizing sentences using italics? I'm not sure its necessary: https://developers.google.com/style/text-formatting

It is very necessary to make sure our readers know they must already have client certs, or they're barking up the wrong tree. Now, I'm open to suggestions on how we go about designating that making-sure. Goog allows emphasis to "draw attention to a specific word or phrase," so that appears to fit.

If its a blocker then wouldn't a caution/notice of some form be better to draw emphasis? https://developers.google.com/style/notices

OK, you've swayed me. I put an Important note in the JWT page to narrow the readership, and so there should be one here. Caution, Notice, and Warning are all too strong -- they won't lose data if they configure cert auth, only time and brain cells.

In other places we use strongly suggest etc. With bold. I suggest we stick with the bold convention for any emphasis.

That having been said, I would also feel better about this being a separate callout in a warning if it is indeed that dire.

rosewms · 2021-05-27T19:28:27Z

docs/src/main/sphinx/security/cert-auth.rst

+
+This feature is *not* appropriate for sites that would need to generate a set of
+client certificates in order to use this authentication method. Starburst
+recommends instead configuring one of the password-based authentication methods,


This sentence reads awkwardly to me, would a list work better since its a grouping of 3 alternatives?

+1. Also, it's better to emphasis what they should do rather than give a visual bump to a no-no.

rosewms

Overall it looks good to me, I left a few suggestions but none of them are blockers.

mosabua

Do we want to add docs for JDBC driver config for certificate auth as well. I think yes and suggest we add a Certificate authentication section in the CLI and JDBC driver docs and in the cert auth doc link there from a "Client support" section

mosabua · 2021-05-27T21:23:00Z

docs/src/main/sphinx/security/cert-auth.rst

+Certificate authentication
+==========================
+
+Trino can be configured to support certified clients seeking validation from a


"certified clients" sounds wrong.

maybe more like (need tweaking)

Trino can be configured to allow clients to authenticate with a certificate that is supplied upon connection

"You can configure Trino to support client-provided certificates validated by the Trino server on initial connection."

mosabua · 2021-05-27T21:24:05Z

docs/src/main/sphinx/security/cert-auth.rst

+==========================
+
+Trino can be configured to support certified clients seeking validation from a
+Trino server. This authentication method is only provided to support sites that


replace "sites" with "users"

mosabua · 2021-05-27T22:34:38Z

docs/src/main/sphinx/security/cert-auth.rst

+
+Trino can be configured to support certified clients seeking validation from a
+Trino server. This authentication method is only provided to support sites that
+have an absolute requirement for client authentication *and already have* client


requirement for client authentication? certificate authentication would be right here I think

mosabua · 2021-05-27T22:34:51Z

docs/src/main/sphinx/security/cert-auth.rst

+Trino can be configured to support certified clients seeking validation from a
+Trino server. This authentication method is only provided to support sites that
+have an absolute requirement for client authentication *and already have* client
+certificates for each client. Sites in this category have an existing PKI


Sites vs Users

mosabua · 2021-05-27T22:35:07Z

docs/src/main/sphinx/security/cert-auth.rst

+Trino server. This authentication method is only provided to support sites that
+have an absolute requirement for client authentication *and already have* client
+certificates for each client. Sites in this category have an existing PKI
+infrastructure, possibly including an onsite CA.


spell out CA maybe

mosabua · 2021-05-27T22:53:37Z

docs/src/main/sphinx/security/cert-auth.rst

+
+    http-server.authentication.type=CERTIFICATE,PASSWORD
+
+The following configuration properties for ``etc/config.properties`` are also


omit "for etc/config.properties"

and drop "also"

I left "also" in unless you object again, because this table is not all properties for cert auth. It does not repeat the auth.type=CERT property above. Thus, the following are also available.

docs/src/main/sphinx/security/cert-auth.rst

m57lyra · 2021-05-27T23:08:47Z

docs/src/main/sphinx/security/cert-auth.rst

+infrastructure, possibly including an onsite CA.
+
+This feature is *not* appropriate for sites that would need to generate a set of
+client certificates in order to use this authentication method. Starburst


This Trino. Not Starburst.

…-auth

mosabua

A bunch of open questions and suggestions added.

docs/src/main/sphinx/installation/cli.rst

docs/src/main/sphinx/security/certificate.rst

docs/src/main/sphinx/installation/cli.rst

docs/src/main/sphinx/security/certificate.rst

Ordinant · 2021-06-30T20:01:23Z

One commit was an inadvertent merge, so this PR won't squash cleanly with git rebase. Closing this PR to make a new one with only the most recent changes.

cla-bot bot added the cla-signed label May 27, 2021

Ordinant requested review from dain, m57lyra, mosabua, rosewms and electrum May 27, 2021 13:35

Ordinant added the docs label May 27, 2021

Add client certificate auth page

70f533f

rosewms reviewed May 27, 2021

View reviewed changes

docs/src/main/sphinx/security/cert-auth.rst Outdated Show resolved Hide resolved

rosewms reviewed May 27, 2021

View reviewed changes

rosewms approved these changes May 27, 2021

View reviewed changes

mosabua requested changes May 27, 2021

View reviewed changes

m57lyra reviewed May 27, 2021

View reviewed changes

Ordinant marked this pull request as draft May 28, 2021 00:42

Ordinant added the WIP label May 28, 2021

Ordinant added 2 commits May 27, 2021 23:12

Add client certificate auth page

5aecbb1

Merge branch 'bw/cert-auth' of github.com:Ordinant/trino into bw/cert…

5072d0e

…-auth

Ordinant removed the WIP label Jun 4, 2021

Ordinant marked this pull request as ready for review June 4, 2021 00:09

Ordinant added 2 commits June 3, 2021 20:15

Small correction.

0d5de78

Single char fix

06814fd

mosabua requested changes Jun 4, 2021

View reviewed changes

More edits from Manfred

f6fb92f

electrum reviewed Jun 25, 2021

View reviewed changes

docs/src/main/sphinx/installation/cli.rst Outdated Show resolved Hide resolved

docs/src/main/sphinx/installation/cli.rst Outdated Show resolved Hide resolved

dain reviewed Jun 30, 2021

View reviewed changes

docs/src/main/sphinx/installation/cli.rst Outdated Show resolved Hide resolved

docs/src/main/sphinx/installation/cli.rst Outdated Show resolved Hide resolved

docs/src/main/sphinx/security/certificate.rst Show resolved Hide resolved

One last set of fixes

c667b61

Ordinant closed this Jun 30, 2021

Ordinant mentioned this pull request Jun 30, 2021

Add certificate auth doc #8438

Merged

Ordinant deleted the bw/cert-auth branch June 30, 2021 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add client certificate auth page #8105

Add client certificate auth page #8105

Ordinant commented May 27, 2021

Ordinant commented May 27, 2021 •

edited

Loading

rosewms May 27, 2021

Ordinant May 27, 2021

m57lyra May 27, 2021

mosabua Jun 4, 2021

rosewms May 27, 2021

Ordinant May 27, 2021 •

edited

Loading

rosewms May 27, 2021

Ordinant May 27, 2021

m57lyra May 27, 2021

rosewms May 27, 2021

m57lyra May 27, 2021

rosewms left a comment

mosabua left a comment

mosabua May 27, 2021

Ordinant May 27, 2021 •

edited

Loading

mosabua May 27, 2021

mosabua May 27, 2021

mosabua May 27, 2021

mosabua May 27, 2021

mosabua May 27, 2021

mosabua May 27, 2021

Ordinant Jun 3, 2021 •

edited

Loading

m57lyra May 27, 2021

mosabua left a comment

Ordinant commented Jun 30, 2021


		http-server.authentication.type=CERTIFICATE,PASSWORD

		The following configuration properties for ``etc/config.properties`` are also

Add client certificate auth page #8105

Add client certificate auth page #8105

Conversation

Ordinant commented May 27, 2021

Ordinant commented May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ordinant May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosewms left a comment

Choose a reason for hiding this comment

mosabua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ordinant May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ordinant Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosabua left a comment

Choose a reason for hiding this comment

Ordinant commented Jun 30, 2021

Ordinant commented May 27, 2021 •

edited

Loading

Ordinant May 27, 2021 •

edited

Loading

Ordinant May 27, 2021 •

edited

Loading

Ordinant Jun 3, 2021 •

edited

Loading