-
Notifications
You must be signed in to change notification settings - Fork 70
Feat: Add HostNameInCertificate for strict encryption #589
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
Adds support for specifying the host name to validate in the server certificate when using strict encryption.
- Introduces
HostNameInCertificate
inConnectSettings
and includes it in the connection string query parameters. - Enhances CLI parsing to accept
-Ns:<hostname>
or-Nstrict:<hostname>
and updates related tests. - Updates documentation and error messages to describe the new switch behavior.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
pkg/sqlcmd/sqlcmd_test.go | Added a test case for strict encryption with hostname in certificate |
pkg/sqlcmd/connect.go | Introduced HostNameInCertificate field and query-string addition |
cmd/sqlcmd/sqlcmd_test.go | Extended CLI argument tests and encryption option tests |
cmd/sqlcmd/sqlcmd.go | Updated flag parsing, regex for strict encryption, and help text |
README.md | Documented the new strict-encryption hostname argument |
Comments suppressed due to low confidence (4)
cmd/sqlcmd/sqlcmd_test.go:112
- Add an assertion to verify that
args.HostNameInCertificate
is correctly populated for the-Ns:myserver.domain.com
case to ensure full parsing coverage.
return args.EncryptConnection == "s:myserver.domain.com"
README.md:133
- Clarify the switch syntax by showing both shorthand and full forms, e.g.
-Ns:<hostname>
or-Nstrict:<hostname>
, to avoid confusion with the bracketed notation.
- To provide the value of the host name in the server certificate when using strict encryption, append the name after a `:` to the `-Ns[trict]`. Example: `-Ns:myhost.domain.com`
pkg/sqlcmd/connect.go:61
- [nitpick] Add a note that this field only applies when
Encrypt
is set tostrict
, to clarify its intended usage.
// The HostNameInCertificate is the name to use for the host in the certificate validation
cmd/sqlcmd/sqlcmd_test.go:588
- [nitpick] Rename the test struct field
hostnameincertificate
tohostnameInCertificate
for consistency with Go naming conventions and the production struct field.
hostnameincertificate string
Change of plans....next iteration, I will change |
i will generate new localization files after merging the driver update change when it goes to main, just to avoid dealing with merge conflicts. |
Fixes #504
Since ODBC sqlcmd uses
-F
for the host name, we are going to follow suit and switch to--vertical
as the parameter to turn on vertical output formatting.sqlcmd -S someserver -Nstrict -F *.mydomain.com