-
Notifications
You must be signed in to change notification settings - Fork 80
Provider Configuration: Support SSL Client Certificates #126
Conversation
Hi, could I get someone to take a look at this PR please? Perhaps @cyrilgdn? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kevinbirch ,
Thanks a lot for your work on this ! I've just tested it and it works fine 👍
I have just one question regarding the rootcert
parameter (see my comments).
Also, could you update the website documentation accordingly please?
postgresql/provider.go
Outdated
Description: "The SSL client certificate private key file path. The file must contain PEM encoded data.", | ||
Required: true, | ||
}, | ||
"rootcert": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if rootcert
must be in this nested block with client cert/key?
If I understood correctly, the CA (rootcert
) is used to verify the server certificate (so authenticate the server).
The client cert is used to authenticate the client.
So you can specify only cert
/key
if you want to authenticate the client or only rootcert
if you want to authenticate the server (and all of them if you want to authenticate both).
So wouldn't it be better to keep cert
and key
in this clientcert
block but have sslrootcert
outside of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that would be totally fine with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've made the requested changes, thank you for your feedback!
postgresql/config.go
Outdated
connStr = fmt.Sprintf(dsnFmt, connValues...) | ||
} | ||
|
||
fmt.Printf("connection string: `%s`", connStr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove it please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Thanks a lot 👍
I'll let you know here when it's released, you can also watch the repository in "Releases only" mode. |
Hi, This feature has been released in v1.6.0 last Friday. |
Fixes: #49
Adds a new optional nested block to the provider configuration for SSL client certificate files paths.
Example:
I tried to match the coding conventions of the project as best as I understand them. I'm happy to adjust the property names if there are preferences from reviewers.