Remove sensitive password logging #931
Conversation
|
Blocked by GH-857. This will render this CL obsolete by 857. You can retarget against v2 instead once we have merged v2 to |
| @@ -94,7 +96,6 @@ export const createApp = async (options: CreateAppOptions): Promise<{ | |||
| userAgent = userAgent.join(", "); | |||
| } | |||
| logger.info("Failed login attempt", | |||
| field("password", cookies.password), | |||
There was a problem hiding this comment.
I think it'd be ok to log the hash of the password versus the expected hash. See #952
There was a problem hiding this comment.
It is ok, but it wouldn't add any information other than "it doesn't match". Therefor it will only make the logs unreadable.
| const certs = await new Promise<pem.CertificateCreationResult>((res, rej): void => { | ||
| pem.createCertificate({ | ||
| selfSigned: true, | ||
| altNames: altNames |
There was a problem hiding this comment.
I don't think this should be automatic. Since the certificate is self signed, your computer won't trust alt names anyway.
There was a problem hiding this comment.
If you decide to trust the self-sigend certificate, the hostname/ip of the certificate still has to match in order to pass for a secure TLS connection.
There was a problem hiding this comment.
Are you positive? We've been using this for a while now without any alt names and no issues have been reported.
There was a problem hiding this comment.
I'm running this behind a proxy (on a different server) which reëncrypts the connection to code-server. The proxy trusts the self-signed certificate of code-server. But it is failing on the hostname verification, because the external ip of code-server is not matching the "localhost" which is in the certificate.
There was a problem hiding this comment.
That's dangerous imo as you can get MITMed easily with a self signed certificate. It's best you generate the certificate yourself and add it as a trusted certificate to the proxy or use HTTP and move code-server onto the proxy server.
There was a problem hiding this comment.
I guess my same reasoning applies in general to code-server generating self signed certificates. 🤔
|
No activity since PR was opened. Closing to keep the PR queue clean. |
|
Reopening to take over. |
|
I think this got deprecated by #835 so I'll just close this off. |
Describe in detail the problem you had and how this PR fixes it
Is there an open issue you can link to?
No