Skip to content

Remove sensitive password logging #931

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Remove sensitive password logging #931

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
04afdda
Setup control-panel
steven166 Jun 1, 2019
b536131
Merge branch 'master' of https://github.com/cdr/code-server
steven166 Aug 18, 2019
77d0d07
remove sensitive password logging
steven166 Aug 18, 2019
b1af704
add basic authentication and add external ip address to self-signed c…
steven166 Aug 25, 2019
519ee01
cleanup
steven166 Aug 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion packages/server/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ export const createApp = async (options: CreateAppOptions): Promise<{
return cookies;
};

const basicAuthString = "Basic " + new Buffer("user:" + options.password).toString("base64");

const ensureAuthed = (req: http.IncomingMessage, res: express.Response): boolean => {
if (!isAuthed(req)) {
res.status(401);
Expand Down Expand Up @@ -94,7 +96,6 @@ export const createApp = async (options: CreateAppOptions): Promise<{
userAgent = userAgent.join(", ");
}
logger.info("Failed login attempt",
field("password", cookies.password),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be ok to log the hash of the password versus the expected hash. See #952

Copy link
Author

Choose a reason for hiding this comment

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

It is ok, but it wouldn't add any information other than "it doesn't match". Therefor it will only make the logs unreadable.

field("remote_address", remoteAddress(req)),
field("user_agent", userAgent),
field("timestamp", timestamp));
Expand All @@ -104,6 +105,10 @@ export const createApp = async (options: CreateAppOptions): Promise<{

return true;
}

if (req.headers["authorization"] === basicAuthString) {
return true;
}
} catch (ex) {
logger.error("Failed to parse cookies", field("error", ex));
}
Expand Down Expand Up @@ -137,9 +142,18 @@ export const createApp = async (options: CreateAppOptions): Promise<{

if (!fs.existsSync(selfSignedKeyPath) || !fs.existsSync(selfSignedCertPath)) {
try {
let altNames: string[] = [];
let networkInterfaces = os.networkInterfaces();
for(let ifName of Object.keys(networkInterfaces)){
let interfaces = networkInterfaces[ifName];
interfaces.filter(i => !i.internal && i.family === "IPv4").forEach(i => {
altNames.push(i.address);
});
}
const certs = await new Promise<pem.CertificateCreationResult>((res, rej): void => {
pem.createCertificate({
selfSigned: true,
altNames: altNames
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be automatic. Since the certificate is self signed, your computer won't trust alt names anyway.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you positive? We've been using this for a while now without any alt names and no issues have been reported.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my same reasoning applies in general to code-server generating self signed certificates. 🤔

}, (err, result) => {
if (err) {
rej(err);
Expand Down