Add user-agent to requests #176

kfdm · 2021-05-27T11:58:09Z

This avoids the default go Go-http-client/1.1 User-Agent in requests.

Ideally I would have this as mosquitto/<version> though I'm unsure how to read the Mosquitto version yet. Any pointers?

iegomez · 2021-05-27T14:26:02Z

Hi, @kfdm!

Thanks for your contribution, but could you tell me why is this needed?

kfdm · 2021-05-27T22:02:59Z

This would affect how it shows up in server logs

time:2021-05-27T12:33:07+00:00  method:POST     uri:/mosquitto/getuser  status:200      size:16    ua:Go-http-client/1.1    reqtime:0.330
time:2021-05-27T12:43:12+00:00  method:POST     uri:/mosquitto/getuser  status:200      size:16    ua:mosquitto    reqtime:0.191

Better to explicitly set the user agent to help with debugging, instead of leaving the generic default.

I still want to see if I can get the proper version in there to make this as mosquitto/<mosquitto version> or perhaps as mosquitto-go-auth/<version>
Do you have any recommendation on that ?

iegomez · 2021-05-28T20:53:22Z

@kfdm We could probably pass the version on plugin initialization and then set it in http/jwt backend.

On the idea itself, I don't feel comfortable hardcoding the user agent. Instead, I'd add options that allow it to be set however the user wants, e.g.:

auth_opt_http_user_agent http-custom-user-agent
auth_opt_http_user_agent_mosquitto_version true 

auth_opt_jwt_user_agent jwt-custom-user-agent
auth_opt_jwt_user_agent_mosquitto_version true

iegomez · 2021-05-28T21:01:08Z

By the way, these are defined at mosquito.h header and provide the full version:

#define LIBMOSQUITTO_MAJOR 2
#define LIBMOSQUITTO_MINOR 0
#define LIBMOSQUITTO_REVISION 8

kfdm · 2021-06-02T12:00:16Z

Thank you for the feedback! 🙇

Though I have not implemented yet, instead of two fields how about a single value?

# No value to default to mosquitto
# custom value to set the agent
auth_opt_http_user_agent http-custom-user-agent
# Or a sentinel value to disable sending the agent all together
auth_opt_http_user_agent false

I am still trying to think of a clean way to get the version number. I feel like there is probably a cleaner way to handle the version than this, so I'm still thinking through it

GoInt data[3] = {LIBMOSQUITTO_MAJOR, LIBMOSQUITTO_MINOR, LIBMOSQUITTO_REVISION};
GoSlice version = {data, 3, 3};

AuthPluginInit(keysSlice, valuesSlice, opts_count, version);

iegomez · 2021-06-02T14:17:27Z

@kfdm I don't agree with defaulting to mosquitto as the User-Agent, we shouldn't set one at all unless the user explicitly tells us so. When the option is not given, we simply shouldn't set the agent and let go-http do whatever it does, e.g. Go-http-client/1.1 in the current version.

Likewise, since we're already adding options, I don't see a reason to impose the appended version: let the user decide if they want to attach it or not.

I am still trying to think of a clean way to get the version number. I feel like there is probably a cleaner way to handle the version than this, so I'm still thinking through it

Mm... maybe a simple sprintf to concatenate and format the version as "x.y.z" and then pass it along as a GoString? Sorry, haven't given it much thought really. 🤷

iegomez · 2021-07-06T04:43:43Z

@kfdm any updates on this? I like the general idea, but would like my last comment to be addressed. Let me know if you're available to do so, or if I should take care of it.

Cheers!

kfdm · 2021-07-06T06:46:53Z

Thank you for checking back. I allowed myself to get distracted by some other things.

I feel like I have to respectfully disagree with you a bit on the default bit. I will yield to you on the specific version number, if you are worried about that from a security perspective.

As an operator, I feel like the least surprising behavior is for an application to provide a user agent. It can be rather frustrating on the infrastructure side, to try to track down a random script that uses that languages' default user-agent. The current behavior is defaulting to sending an incorrect agent (from the perspective of an operator)

iegomez · 2021-07-06T13:11:05Z

@kfdm Yeah, you might be right. I don't really feel very strongly about it, maybe the fact that it'd be a change that would probably go unnoticed but could in fact surprise users (hey, why is there this user agent all of a sudden?) had me leaning towards not changing it by default. But let's just default to mosquitto and be explicit about it in the docs.

kfdm · 2021-07-08T09:39:06Z

I've rebased to make sure there are no issues. There's not a specific changelog file in the repository itself, so I'm not sure if there's a specific place to call out the update to the default user-agent

iegomez

Thanks for rebasing! Will you add Mosquitto's version? If so, please also add tests for both backends with and without having versions passed along.

iegomez · 2021-07-08T12:49:33Z

backends/http.go

@@ -79,6 +80,11 @@ func NewHTTP(authOpts map[string]string, logLevel log.Level) (HTTP, error) {
 		missingOpts += " http_aclcheck_uri"
 	}

+	http.UserAgent = "mosquitto"


Can we make the default a constant? Also, feel free not to export it, i.e. userAgent instead of UserAgent. I just haven't gotten to refactor http like I did with jwt backend.

iegomez · 2021-07-08T12:50:07Z

backends/jwt_remote.go

@@ -80,6 +81,11 @@ func NewRemoteJWTChecker(authOpts map[string]string, options tokenOptions) (jwtC
 		missingOpts += " jwt_aclcheck_uri"
 	}

+	checker.userAgent = "mosquitto"


iegomez · 2021-07-08T12:53:45Z

I've rebased to make sure there are no issues. There's not a specific changelog file in the repository itself, so I'm not sure if there's a specific place to call out the update to the default user-agent

I think adding to the README and documenting Mosquitto's version once you add it is enough, then I can mention the change ion the next release as I usually do.

kfdm · 2021-07-10T07:04:00Z

I think I'll leave off the version number for now. Can always try to attempt it again, but my earlier attempts to try to get the version number from the mosquitto side were not as clean as I had hoped.

Both http.go and jwt_remote.go are in the same backend package, so I put defaultUserAgent with the rest of the constants.

Would you prefer me to rebase everything into a single commit, or will you do that as part of the final merge?

iegomez · 2021-07-10T16:02:25Z

Cool, defaultUserAgent looks good. And don't worry about rebasing, I'll give the version a try and add a test before squashing and merging.

Thanks again!

iegomez · 2021-07-11T23:37:42Z

@kfdm I added the version to the default user agent in #182.
Let me know what you think.

By the way, feel free to rebase/merge the version changes in your branch so that one gets merged instead of mine.

kfdm · 2021-07-12T00:19:13Z

That looks great! I'm not picky about getting credit for it. My work was quite simple. I'm also fine with you closing this PR and merging yours if that makes it easiest for you :)

iegomez · 2021-07-12T00:36:31Z

It was your idea, @kfdm, I just added the version, so credit where credit is due.
I didn't know how to commit on your branch (is that even possible with PRs from forks?) so I opened a separate PR, but again, feel free to rebase and I'll merge yours. 😉

Add more room for extended versions.

kfdm · 2021-07-12T01:25:59Z

Ok, I think I succeeded in rebasing correctly. It's now in two commits.
The first was my original few commits squashed into one
The second is your changes to bring in the version and all the test cases.

Git kept trying to assign the edit to me XD so I think I fixed that with git commit --amend --author=".." --date="$(date -R)"

Let me know if it still looks ok.

iegomez · 2021-07-12T01:49:59Z

Cool then, let's merge it. Thanks, Paul!

kfdm requested a review from iegomez as a code owner May 27, 2021 11:58

kfdm force-pushed the user-agent branch from 4ecdf20 to ea4fa46 Compare July 8, 2021 09:36

iegomez reviewed Jul 8, 2021

View reviewed changes

kfdm and others added 2 commits July 12, 2021 10:16

Add default 'mosquitto' user-agent to http/jwt requests

7b80257

Add libmosquitto version ofr http and jwt remote user agent.

d813f0e

Add more room for extended versions.

kfdm force-pushed the user-agent branch from a567420 to d813f0e Compare July 12, 2021 01:23

iegomez approved these changes Jul 12, 2021

View reviewed changes

iegomez merged commit 5dc063f into iegomez:master Jul 12, 2021

kfdm deleted the user-agent branch July 12, 2021 03:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user-agent to requests #176

Add user-agent to requests #176

kfdm commented May 27, 2021

iegomez commented May 27, 2021

kfdm commented May 27, 2021

iegomez commented May 28, 2021

iegomez commented May 28, 2021

kfdm commented Jun 2, 2021

iegomez commented Jun 2, 2021

iegomez commented Jul 6, 2021

kfdm commented Jul 6, 2021 •

edited

Loading

iegomez commented Jul 6, 2021

kfdm commented Jul 8, 2021

iegomez left a comment

iegomez Jul 8, 2021

iegomez Jul 8, 2021

iegomez commented Jul 8, 2021

kfdm commented Jul 10, 2021

iegomez commented Jul 10, 2021

iegomez commented Jul 11, 2021 •

edited

Loading

kfdm commented Jul 12, 2021

iegomez commented Jul 12, 2021

kfdm commented Jul 12, 2021

iegomez commented Jul 12, 2021

Add user-agent to requests #176

Add user-agent to requests #176

Conversation

kfdm commented May 27, 2021

iegomez commented May 27, 2021

kfdm commented May 27, 2021

iegomez commented May 28, 2021

iegomez commented May 28, 2021

kfdm commented Jun 2, 2021

iegomez commented Jun 2, 2021

iegomez commented Jul 6, 2021

kfdm commented Jul 6, 2021 • edited Loading

iegomez commented Jul 6, 2021

kfdm commented Jul 8, 2021

iegomez left a comment

Choose a reason for hiding this comment

iegomez Jul 8, 2021

Choose a reason for hiding this comment

iegomez Jul 8, 2021

Choose a reason for hiding this comment

iegomez commented Jul 8, 2021

kfdm commented Jul 10, 2021

iegomez commented Jul 10, 2021

iegomez commented Jul 11, 2021 • edited Loading

kfdm commented Jul 12, 2021

iegomez commented Jul 12, 2021

kfdm commented Jul 12, 2021

iegomez commented Jul 12, 2021

kfdm commented Jul 6, 2021 •

edited

Loading

iegomez commented Jul 11, 2021 •

edited

Loading