Skip to content

Conversation

@mwojtyczka
Copy link

I added https support for the proxy. I used the proxy in my project but to make it really useful I had to add HTTPS. When using it over HTTP the jwt token can easily be compromised. I updated the documentation accordingly. I did not update the docker files though but that can be easily done (I guess by adding separate docker files for the https or by parametrization).

@sokil
Copy link
Member

sokil commented Jan 25, 2019

Thank you for contributing.

Currently i expect that this proxy will be behind nginx, where you may configure balancing througn few proxy instance and add https support. Why you can't use nginx?

Update of dockerfiles is not required because any cli parameter may be passed in compose file or in run command:

docker run -p 80:80 gometric/statsd-http-proxy:latest --verbose --tls-cert="..."

JWT token is public information, there are no payload there, and it used just to sign expiration time. Is this really critical?

Will review your code soon.

@mwojtyczka
Copy link
Author

Thank you! Yes you can certainly pass the docker parameters, just make sure the certificate is somehow provided (by copying to the container or by volume sharing). A description might be sufficient.

For the HTTPS you can certainly place it behing the ngnix but for a simple use cases it is an overkill in my opinion. You can easily support the https with the current libraries without additional complexity. I personally always use https whenever possible. As a rule of thumb I do not want anyone to be able to intercept my communication, no matter what it is.

main.go Outdated
if ( len(*tlsCert) > 0 && len(*tlsKey) > 0) {
// start https server
err := s.ListenAndServeTLS(*tlsCert, *tlsKey)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

err handle may be moved outside if

Copy link
Author

Choose a reason for hiding this comment

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

Corrected

@@ -0,0 +1,16 @@
#!/bin/sh

# This server start listening connections by HTTP and pass it to StatsD by UDP
Copy link
Member

Choose a reason for hiding this comment

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

copy-paste. it's preferred to use one script to start server in different modes. It may ask yser which wariant to start.

Also you specify certificate and key, but they not present.
You may pre-generate it and commit, or to add this files to .gitignore and generate them on first run.

This cli is not friendly

Copy link
Author

Choose a reason for hiding this comment

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

Done, added cert and key generation, as well as ignored them in the .gitignore.

</head>
<body>

You are running demo of StatsD HTTPS Proxy. Please, start <b>statsdHttpProxy.sh</b> for handling HTTP requests and
Copy link
Member

Choose a reason for hiding this comment

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

copy-paste. maybe better will be to use raddio buttons to select server. And script name statsdHttpProxy.sh is invalid in help message. But i think all server modes must be started from one bash

Copy link
Author

Choose a reason for hiding this comment

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

Added radio button.


```javascript
$.ajax({
url: 'https://127.0.0.1:433/count/some.key.name',
Copy link
Member

Choose a reason for hiding this comment

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

this samples differ only by url. Everyone know what it is https))

Copy link
Author

Choose a reason for hiding this comment

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

removed, merged into the existing index.html

.gitignore Outdated

# ide files
.idea/**
/**/.idea/**
Copy link
Member

Choose a reason for hiding this comment

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

  1. add new line on end of file
  2. ide rules must be in your global gitignore in home dir

Copy link
Author

Choose a reason for hiding this comment

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

Done.

demo/index.html Outdated
<body>

You are running demo of StatsD HTTP Proxy. Please, start <b>statsdHttpProxy.sh</b> for handling HTTP requests and
You are running demo of StatsD HTTP(s) Proxy. Please, start <b>statsdHttp(s)Proxy.sh</b> for handling HTTP(s) requests and
Copy link
Member

@sokil sokil Jan 28, 2019

Choose a reason for hiding this comment

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

now i can't just copy file from browser and place to console )

Copy link
Author

Choose a reason for hiding this comment

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

Corrected

--statsd-host=127.0.0.1 \
--statsd-port=8125 \
--jwt-secret=somesecret \
--metric-prefix=prefix.subprefix No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

new line.

Copy link
Author

Choose a reason for hiding this comment

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

done

# This server start listening connections by HTTPS and pass it to StatsD by UDP

# generate self-signed cert and key with default subject
#openssl req -x509 -nodes -days 358000 -newkey rsa:2048 -keyout key.pem -out cert.pem -subj "/C=PL/ST=test/L=test/O=test/OU=test/CN=test"
Copy link
Member

Choose a reason for hiding this comment

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

you may cehck if file not found and generate in automatically

Copy link
Author

Choose a reason for hiding this comment

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

Corrected. Generating the credentials only if they do not exist.

main.go Outdated
}

if err != nil {
log.Fatal(err)
Copy link
Member

@sokil sokil Jan 28, 2019

Choose a reason for hiding this comment

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

i think there must be some tabs instead of spaces. run go fmt

Copy link
Author

Choose a reason for hiding this comment

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

done

main.go Outdated
BuildDate,
)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

new line

Copy link
Author

Choose a reason for hiding this comment

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

done

README.md Outdated
--verbose \
--http-host=127.0.0.1 \
--http-port=433 \
--tls-cert=cert.pem \
Copy link
Member

@sokil sokil Jan 28, 2019

Choose a reason for hiding this comment

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

My GoLand IDE configured to use tabs instead of spaces for indentation due to:

Indentation
We use tabs for indentation and gofmt emits them by default. Use spaces only if you must.

You are using spaces)

Copy link
Author

Choose a reason for hiding this comment

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

done

.glide/

# keys
*.pem
Copy link
Member

@sokil sokil Jan 28, 2019

Choose a reason for hiding this comment

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

pem ignored in all project or just in demo dir?

Copy link
Author

@mwojtyczka mwojtyczka Jan 29, 2019

Choose a reason for hiding this comment

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

I would generally exlude pem files from the whole project. They should never be checked in. For demo it will be generated on the fly.

@sokil sokil merged commit d73aff2 into GoMetric:master Jan 29, 2019
@sokil
Copy link
Member

sokil commented Jan 29, 2019

Merged in https://github.com/GoMetric/statsd-http-proxy/releases/tag/0.9.0.

Thank you for your work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants