Skip to content
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

Add NTML Auth Option for HTTP #1639

Merged
merged 8 commits into from
Jun 14, 2022
Merged

Add NTML Auth Option for HTTP #1639

merged 8 commits into from
Jun 14, 2022

Conversation

christopherpickering
Copy link
Contributor

@christopherpickering christopherpickering commented May 12, 2022

Description

Fixes #1634

This pr is to add an option for http(s) authentication against IIS servers that are using the "Windows Authentication" feature. It turns out there is already a nice npm package that handles this very simply httpntml.

Type of change

  • New feature (non-breaking change which adds functionality)

This change will add an option in the monitor editor for http, in the auth section, for "NTML Authentication".

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots

@christopherpickering
Copy link
Contributor Author

@louislam What is your preferred way to add this in? I was initially thinking add a dropdown for auth method (as there is also negotiate which someone might ask for one day).
image

This would add another db field to know what type of auth.

But in the code,

if (this.basic_auth_user) {
    basicAuthHeader = {
        "Authorization": "Basic " + this.encodeBase64(this.basic_auth_user, this.basic_auth_pass),
    };
}

it is checking if there is a username and then trying to login. So I probably cannot share the columns?
Or, in the migration sql, should I add a column (auth_method), and add a "basic" if a "basic_auth_user" is populated, to backfill for existing monitors?

And going forward, the dropdown would populate that column and it could be used to determine what type of auth to use.

What do you think?

@louislam
Copy link
Owner

I think we should find a suitable library first.

Currently http(s) monitor is fully based on axios http client, which means if you want to add a NTML, it have to do it on axios.

The library you mentioned in #1634 is not a node module and it looks like for browser only, I don't think it could work with axios.

So it is better to test libraries and find the best one before start.

@christopherpickering
Copy link
Contributor Author

Ops, I forgot to update that, there is actually a node module https://www.npmjs.com/package/httpntlm I tested out and it works very simply. I'll make a proof of concept.

Basic:

var httpntlm = require('httpntlm');

httpntlm.get({
    url: "https://example.net",
    username: 'me',
    password: 'pa$$',
    workstation: '',
    domain: ''
}, function (err, res){
    if(err) {
        console.log(err);
        return
    }

    console.log(res.headers);
    console.log(res.body);
    console.log(res.statusCode)
});

@christopherpickering
Copy link
Contributor Author

After struggling w/ parameters from axios and httpntlm not matching, I found axios-ntlm, which is an axios implementation of ntlm... which made it pretty simple.

What do you think about the database file? It seems like columns cannot be renamed easily in sqlite, so I left the basic_auth.. column names alone, but they are shared now between auth types.

Anyone who had an basic_auth_user now get the method of basic selected for their previously created monitors.

Going forward, the auth type would be based on the dropdown.

@christopherpickering christopherpickering marked this pull request as ready for review May 13, 2022 18:01
Copy link
Contributor

@Computroniks Computroniks left a comment

Choose a reason for hiding this comment

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

Formatting and JSDoc comments look good. 👍

@christopherpickering
Copy link
Contributor Author

@Computroniks thank you!

@louislam louislam added this to the 1.17.0 milestone May 22, 2022
@louislam louislam merged commit 0e8f6d2 into louislam:master Jun 14, 2022
@christopherpickering
Copy link
Contributor Author

Thanks!

@christopherpickering
Copy link
Contributor Author

christopherpickering commented Jun 14, 2022

I should note - workstation is a required field for axios-ntml. If it is left blank there will be an error "Cannot read property 'length' of null".

I think this can be fixed by workstation: this.authWorkstation || "Uptime-Kuma", I will pr as it is a pain in the neck to fill in a fairly useless field.

<label for="method" class="form-label">{{ $t("Method") }}</label>
<select id="method" v-model="monitor.authMethod" class="form-select">
<option :value="null">
None
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add translation

None
</option>
<option value="basic">
Basic
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add translation

@christopherpickering
Copy link
Contributor Author

@Saibamen my bad, thanks, I'll put it in a separate pr.

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.

NTML Windows Authentication
4 participants