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

adds host and scheme validation for access token providers #1051

Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added

- Added the ability to configure the underlying transport in Go. #1003
- Adds hostname and protocol validation in authentication. #1051

### Changed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,63 @@ public async Task BaseBearerTokenAuthenticationProviderSetsBearerHeader()
Assert.Equal("Authorization", testRequest.Headers.First().Key); // First element is Auth header
Assert.Equal($"Bearer {expectedToken}", testRequest.Headers.First().Value); // First element is Auth header
}

[Theory]
[InlineData("https://graph.microsoft.com",true)]// PASS
[InlineData("https://graph.microsoft.us/v1.0/me",true)]// PASS as we don't look at the path segment
[InlineData("https://test.microsoft.com",false)]// Fail
[InlineData("https://grAph.MicrosofT.com",true)] // PASS since we don't care about case
[InlineData("https://developer.microsoft.com",false)] // Failed
public void AllowedHostValidatorValidatesUrls(string urlToTest, bool expectedResult)
{
// Test through the constructor
// Arrange
var whiteList = new[] { "graph.microsoft.com", "graph.microsoft.us"};
var validator = new AllowedHostsValidator(whiteList);

// Act
var validationResult = validator.IsUrlHostValid(new Uri(urlToTest));

// Assert
Assert.Equal(expectedResult, validationResult);
Assert.Contains(whiteList[0], validator.AllowedHosts);
Assert.Contains(whiteList[1], validator.AllowedHosts);


// Test through the setter
// Arrange
var emptyValidator = new AllowedHostsValidator
{
AllowedHosts = whiteList // set the validator through the property
};

// Act
var emptyValidatorResult = emptyValidator.IsUrlHostValid(new Uri(urlToTest));

// Assert
Assert.Equal(emptyValidatorResult, validationResult);
Assert.Contains(whiteList[0], emptyValidator.AllowedHosts);
Assert.Contains(whiteList[1], emptyValidator.AllowedHosts);
}


[Theory]
[InlineData("https://graph.microsoft.com")]// PASS
[InlineData("https://graph.microsoft.us/v1.0/me")]// PASS
[InlineData("https://test.microsoft.com")]// PASS
[InlineData("https://grAph.MicrosofT.com")] // PASS
[InlineData("https://developer.microsoft.com")] // PASS
public void AllowedHostValidatorAllowsAllUrls(string urlToTest)
{
// Test through the constructor
// Arrange
var validator = new AllowedHostsValidator();

// Act
var validationResult = validator.IsUrlHostValid(new Uri(urlToTest));

// Assert
Assert.True(validationResult);
Assert.Empty(validator.AllowedHosts);
}
}
53 changes: 53 additions & 0 deletions abstractions/dotnet/src/authentication/AllowedHostsValidator.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// ------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All Rights Reserved. Licensed under the MIT License. See License in the project root for license information.
// ------------------------------------------------------------------------------

using System;
using System.Collections.Generic;
using System.Linq;

namespace Microsoft.Kiota.Abstractions.Authentication
{
/// <summary>
/// Validator for handling allowed hosts for authentication
/// </summary>
public class AllowedHostsValidator
{
private HashSet<string> _allowedHosts;

/// <summary>
/// The <see cref="AllowedHostsValidator"/> constructor
/// </summary>
/// <param name="validHosts"> Collection of valid Hosts</param>
public AllowedHostsValidator(IEnumerable<string> validHosts = null)
{
_allowedHosts = new HashSet<string>(validHosts ?? Array.Empty<string>(), StringComparer.OrdinalIgnoreCase);
}

/// <summary>
/// Gets/Sets the collection of allowed hosts for the configurator
/// </summary>
public IEnumerable<string> AllowedHosts
{
get => _allowedHosts.AsEnumerable();
set
{
if(value is null) throw new ArgumentNullException(nameof(value));
_allowedHosts = new HashSet<string>(value.Where(x => !string.IsNullOrEmpty(x)), StringComparer.OrdinalIgnoreCase);
}
}

/// <summary>
/// Validates that the given Uri is valid
/// </summary>
/// <param name="uri">The <see cref="Uri"/> to validate</param>
/// <returns>
/// true - if the host is in the <see cref="AllowedHosts"/>. If <see cref="AllowedHosts"/> is empty, it will return true for all urls.
/// false - if the <see cref="AllowedHosts"/> is not empty and the host is not in the list
/// </returns>
public bool IsUrlHostValid(Uri uri)
{
return !_allowedHosts.Any() || _allowedHosts.Contains(uri.Host);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,8 @@ public async Task AuthenticateRequestAsync(RequestInformation request, Cancellat
if(!request.Headers.ContainsKey(AuthorizationHeaderKey))
{
var token = await AccessTokenProvider.GetAuthorizationTokenAsync(request.URI, cancellationToken);
if(string.IsNullOrEmpty(token))
throw new InvalidOperationException("Could not get an authorization token");
request.Headers.Add(AuthorizationHeaderKey, $"Bearer {token}");
if(!string.IsNullOrEmpty(token))
request.Headers.Add(AuthorizationHeaderKey, $"Bearer {token}");
}
}
}
49 changes: 49 additions & 0 deletions abstractions/go/authentication/allowed_hosts_validator.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package authentication

import (
u "net/url"
"strings"
)

// AllowedHostsValidator Maintains a list of valid hosts and allows authentication providers to check whether a host is valid before authenticating a request
type AllowedHostsValidator struct {
validHosts map[string]bool
}

// NewAllowedHostsValidator creates a new AllowedHostsValidator object with provided values.
func NewAllowedHostsValidator(validHosts []string) AllowedHostsValidator {
result := AllowedHostsValidator{}
result.SetAllowedHosts(validHosts)
return result
}

// GetAllowedHosts returns the list of valid hosts.
func (v *AllowedHostsValidator) GetAllowedHosts() map[string]bool {
hosts := make(map[string]bool, len(v.validHosts))
for host := range v.validHosts {
hosts[host] = true
}
return hosts
}

//SetAllowedHosts sets the list of valid hosts.
func (v *AllowedHostsValidator) SetAllowedHosts(hosts []string) {
v.validHosts = make(map[string]bool, len(hosts))
if len(hosts) > 0 {
for _, host := range hosts {
v.validHosts[strings.ToLower(host)] = true
}
}
}

// IsValidHost returns true if the host is valid.
func (v *AllowedHostsValidator) IsUrlHostValid(uri *u.URL) bool {
if uri == nil {
return false
}
host := uri.Host
if host == "" {
return false
}
return v.validHosts[strings.ToLower(host)]
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,9 @@ func (provider *BaseBearerTokenAuthenticationProvider) AuthenticateRequest(reque
if err != nil {
return err
}
if token == "" {
return errors.New("could not get an authorization token")
if token != "" {
request.Headers[authorizationHeader] = "Bearer " + token
}
request.Headers[authorizationHeader] = "Bearer " + token
}

return nil
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package com.microsoft.kiota.authentication;

import java.net.URI;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;

import javax.annotation.Nonnull;

/** Maintains a list of valid hosts and allows authentication providers to check whether a host is valid before authenticating a request */
public class AllowedHostsValidator {
private HashSet<String> validHosts;
/**
* Creates a new AllowedHostsValidator.
* @param validHosts The list of valid hosts.
*/
public AllowedHostsValidator(@Nonnull final String... allowedHosts) {
this.setAllowedHosts(allowedHosts);
}

/**
* Gets the allowed hosts. Read-only.
* @return the allowed hosts.
*/
@Nonnull
public Set<String> getAllowedHosts() {
return Collections.unmodifiableSet(this.validHosts);
}
/**
* Sets the allowed hosts.
* @param allowedHosts the allowed hosts.
*/
public void setAllowedHosts(@Nonnull final String... allowedHosts) {
validHosts = new HashSet<String>();
if(allowedHosts != null) {
for (final String host : allowedHosts) {
if (host != null && !host.isEmpty())
validHosts.add(host.trim().toLowerCase());
}
}
}

/**
* Checks if the provided host is allowed.
* @param uri the uri to check the host for.
* @return true if the host is allowed, false otherwise.
*/
public boolean isUrlHostValid(@Nonnull final URI uri) {
return validHosts.isEmpty() || validHosts.contains(uri.getHost().trim().toLowerCase());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ public CompletableFuture<Void> authenticateRequest(final RequestInformation requ
}
return this.accessTokenProvider.getAuthorizationToken(targetUri)
.thenApply(token -> {
if(token == null || token.isEmpty()) {
throw new UnsupportedOperationException("Could not get an authorization token", null);
if(token != null && !token.isEmpty())) {
// Not an error, just no need to authenticate as we might have been given an external URL from the main API (large file upload, etc.)
request.headers.put(authorizationHeaderKey, "Bearer " + token);
}
request.headers.put(authorizationHeaderKey, "Bearer " + token);
return null;
});
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/** Maintains a list of valid hosts and allows authentication providers to check whether a host is valid before authenticating a request */
export class AllowedHostsValidator {
private allowedHosts: Set<string>;
/**
* Creates a new AllowedHostsValidator object with provided values.
* @param allowedHosts A list of valid hosts. If the list is empty, all hosts are valid.
*/
public constructor(allowedHosts: Set<string> = new Set<string>()) {
this.allowedHosts = allowedHosts ?? new Set<string>();
}
/**
* Gets the list of valid hosts. If the list is empty, all hosts are valid.
* @returns A list of valid hosts. If the list is empty, all hosts are valid.
*/
public getAllowedHosts(): string[] {
return Array.from(this.allowedHosts);
}
/**
* Sets the list of valid hosts. If the list is empty, all hosts are valid.
* @param allowedHosts A list of valid hosts. If the list is empty, all hosts are valid.
*/
public setAllowedHosts(allowedHosts: Set<string>): void {
this.allowedHosts = allowedHosts;
}
/**
* Checks whether the provided host is valid.
* @param url The url to check.
*/
public isUrlHostValid(url: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use a class instead of simply exporting a util function?

Copy link
Member Author

Choose a reason for hiding this comment

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

because we need something to hold the list of valid hostnames?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. I think that would be necessary for Java or dot net.

Currently we maintain this as util functions:

https://github.com/microsoftgraph/msgraph-sdk-javascript/blob/3eab6727853b1977bd8046fa3bab8e95dafe6f40/src/GraphRequestUtil.ts#L68

https://github.com/microsoftgraph/msgraph-sdk-javascript/blob/9441382f7963fa782cd30a103af2632d7585e0a0/src/Constants.ts#L28

Related to the comment below we could validate the url using a single util function.

Copy link
Member Author

Choose a reason for hiding this comment

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

the main difference being the list of hosts is maintained on a global context object, which is a mix of everything (request options, hosts, etc...) vs a self contained (encapsulated) object.
https://github.com/microsoftgraph/msgraph-sdk-javascript/blob/3eab6727853b1977bd8046fa3bab8e95dafe6f40/src/IContext.ts#L29

I do have a preference for encapsulation over a mix "context" object. What do you think?

Copy link
Contributor

@nikithauc nikithauc Jan 28, 2022

Choose a reason for hiding this comment

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

I am with you about not storing hosts in context object.

When I thinking of the flow for Graph JS SDK we would go

Client.init({
authProvider= new AzureAuthenticationProvider(credential, user_custom_hosts);
});
  1. The user_custom_hosts would not contain the Graph hosts.

  2. At the time of initialization I would append the Graph hosts to this user_custom_hosts.

Is the use case valid?
If that is the case, the allowed hosts should be public property so that it can be modified.

In that case, the allowed hosts validation can be a util function which can be passed the allowed hosts. We won't be storing it in the Context.

What I missing how is the AllowHostsValidator class going to be accessible to Graph JS SDK to store its hosts and then passed to the AuthenticationProvider?

We can continue this discussion in #1063 and make more updates accordingly. This conversation need not block this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

added the getter

Copy link
Member Author

Choose a reason for hiding this comment

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

CC @Ndiritu for PHP, sorry I already had merged your PR

Copy link
Member Author

Choose a reason for hiding this comment

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

also CC @samwelkanda for the ongoing work in #925

Copy link
Contributor

@nikithauc nikithauc Jan 31, 2022

Choose a reason for hiding this comment

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

right, we're probably missing a getter to the allowed hosts validator in the access token provider interface for the chain to be complete to people can access things afterwards.

How about have a public function AppendHosts in the hosts validator class?

Copy link
Member Author

Choose a reason for hiding this comment

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

if we add an append host (which could be done by Get, append to the result, set) we'd also need to add a remove host method for consistency. All of that can already be done with the current API surface, I don't think it'll add much value to the current offering.

if(!url) return false;
if(this.allowedHosts.size === 0) return true;
const schemeAndRest = url.split("://");
if(schemeAndRest.length >= 2) {
const rest = schemeAndRest[1];
if(rest) {
return this.isHostAndPathValid(rest);
}
} else if (!url.startsWith("http")) {
// protocol relative URL domain.tld/path
return this.isHostAndPathValid(url);
}
//@ts-ignore
if(window && window.location && window.location.host) {
// we're in a browser, and we're using paths only ../path, ./path, /path, etc.
//@ts-ignore
return this.allowedHosts.has((window.location.host as string)?.toLowerCase());
}
return false;
}
private isHostAndPathValid(rest: string): boolean {
const hostAndRest = rest.split("/");
if(hostAndRest.length >= 2) {
const host = hostAndRest[0];
if(host) {
return this.allowedHosts.has(host.toLowerCase());
}
}
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,12 @@ export class BaseBearerTokenAuthenticationProvider implements AuthenticationProv
}
if (!request.headers?.has(BaseBearerTokenAuthenticationProvider.authorizationHeaderKey)) {
const token = await this.accessTokenProvider.getAuthorizationToken(request.URL);
if (!token) {
throw new Error('Could not get an authorization token');
}
if (!request.headers) {
request.headers = new Map<string, string>();
}
request.headers?.set(BaseBearerTokenAuthenticationProvider.authorizationHeaderKey, `Bearer ${token}`);
if(token) {
request.headers?.set(BaseBearerTokenAuthenticationProvider.authorizationHeaderKey, `Bearer ${token}`);
}
}
}
}
1 change: 1 addition & 0 deletions abstractions/typescript/src/authentication/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export * from './accessTokenProvider';
export * from './allowedHostsValidator';
export * from './authenticationProvider';
export * from './anonymousAuthenticationProvider';
export * from './baseBearerTokenAuthenticationProvider';
Loading