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.

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

Jump to bottom

Update Security Samples #301

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Update Security Samples #301

wants to merge 2 commits into from

Conversation

TimHess
Copy link
Member

@TimHess TimHess commented Jul 14, 2023

Use minimal API, add actuators, only listen over HTTPS, improve usability

TODO: update integration test, help users understand how to enable UAA as a user store for the tile

bart-vmware
bart-vmware reviewed Jul 18, 2023
View reviewed changes
Security/src/CloudFoundrySingleSignon/Controllers/HomeController.cs Outdated
{
_httpContextAccessor = httpContextAccessor;
Copy link
Member

Choose a reason for hiding this comment

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

This field is never used. Can it be removed (including the constructor parameter)?

Security/src/CloudFoundrySingleSignon/Program.cs Outdated
}
options.AccessDeniedPath = new PathString("/Home/AccessDenied");
})
// .AddCloudFoundryOAuth(builder.Configuration)
Copy link
Member

Choose a reason for hiding this comment

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

Do you think this commented-out code line should be removed? If not, a comment indicating its purpose would help.

Security/src/CloudFoundrySingleSignon/README-UAA.md Outdated

ASP.NET Core sample app illustrating how to make use of the Steeltoe [CloudFoundry External Security Provider](https://docs.steeltoe.io/api/v3/security/) for Authentication and Authorization against a CloudFoundry OAuth2 security service (e.g. [UAA Server](https://github.com/cloudfoundry/uaa) or [Tanzu Single Signon Service](https://docs.pivotal.io/p-identity/)).

> NOTE: For simplicity, we've moved the [instructions for utilizing the SSO Tile](README.md) with this sample
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify this sentence? To/into/included/copied?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm removing everything related to the direct UAA approach, I don't think that's a real use case

Security/src/CloudFoundrySingleSignon/README-UAA.md Outdated
After authenticating, add a new `user` and `group` to the UAA Server database. Do NOT change the group name: `testgroup` as it is used for policy based authorization in the sample application. Feel free to change the username and password to anything you would like.

1. uaac group add testgroup
1. uaac user add dave --given_name Dave --family_name Tillman --emails dave@testcloud.com --password Password1!
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps replace 'dave' with something else? (multiple places in this file and others)

Security/src/CloudFoundrySingleSignon/README-UAA.md Outdated

### Add User-Provided Service with OAuth Details

Last, we will create a user-provide service that includes the appropriate UAA server configuration data. Use the sample below to pass the parameters directly to the `cf cups` command, replacing `<YOUR-CLOUDFOUNDRY-SYSTEM-DOMAIN>` with your domain.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Last, we will create a user-provide service that includes the appropriate UAA server configuration data. Use the sample below to pass the parameters directly to the `cf cups` command, replacing `<YOUR-CLOUDFOUNDRY-SYSTEM-DOMAIN>` with your domain.
Last, we will create a user-provided service that includes the appropriate UAA server configuration data. Use the sample below to pass the parameters directly to the `cf cups` command, replacing `<YOUR-CLOUDFOUNDRY-SYSTEM-DOMAIN>` with your domain.

Security/src/CloudFoundrySingleSignon/README-UAA.md Outdated
* `dotnet publish -f netcoreapp3.1 -r win10-x64`
1. Push the app using the appropriate manifest.
* `cf push -f manifest.yml -p bin/Debug/netcoreapp3.1/linux-x64/publish`
* `cf push -f manifest-windows.yml -p bin/Debug/netcoreapp3.1/win10-x64/publish`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `cf push -f manifest-windows.yml -p bin/Debug/netcoreapp3.1/win10-x64/publish`
* `cf push -f manifest-windows.yml -p bin/Debug/netcoreapp3.1/win-x64/publish`

Security/src/CloudFoundrySingleSignon/README-UAA.md Outdated
* `cf push -f manifest.yml -p bin/Debug/netcoreapp3.1/linux-x64/publish`
* `cf push -f manifest-windows.yml -p bin/Debug/netcoreapp3.1/win10-x64/publish`

> Note: The provided manifest(s) will create an app named `single-signon` and attempt to bind it to the user-provided service `myOAuthService`.
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this was renamed to mySSOService in other places. Should this be renamed accordingly?

Suggested change
> Note: The provided manifest(s) will create an app named `single-signon` and attempt to bind it to the user-provided service `myOAuthService`.
> Note: The provided manifest(s) will create an app named `single-signon` and attempt to bind it to the user-provided service `mySSOService`.

Security/src/CloudFoundrySingleSignon/Startup.cs Outdated
Comment on lines 38 to 39
var options = serviceProvider.GetService<IOptions<CertificateOptions>>();
if (options?.Value.Certificate != null)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think GetService for an IOptions<> ever returns null. If there's no configuration, you'll get a default instance.

Suggested change
var options = serviceProvider.GetService<IOptions<CertificateOptions>>();
if (options?.Value.Certificate != null)
var options = serviceProvider.GetRequiredService<IOptions<CertificateOptions>>();
if (options.Value.Certificate != null)

The same suggestion applies to other places in this PR, which I haven't marked.

Security/src/CloudFoundrySingleSignon/Program.cs Outdated
options.AddPolicy("testgroup1", policy => policy.RequireClaim("scope", "testgroup1"));
});

// Add Redis to allow scaling beyond a single instance
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Add Redis to allow scaling beyond a single instance
// Uncomment the code below to add Redis to allow scaling beyond a single instance

Security/src/CloudFoundrySingleSignon/Controllers/HomeController.cs Outdated
}
else
{
serviceHostname = serviceName + hostName.Substring(indx);
serviceHostname = string.Concat(serviceName, hostName.AsSpan(indexOfHost));
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, did you get this suggestion from an analyzer? Resharper suggests using range indexer syntax.

Suggested change
serviceHostname = string.Concat(serviceName, hostName.AsSpan(indexOfHost));
serviceHostname = $"{serviceName}{hostName[indx..]}";

Copy link
Member Author

Choose a reason for hiding this comment

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

This was probably a Visual Studio suggestion, Resharper's preference is fine with me

@TimHess
Update Security Samples
77ee2e2 
Use minimal API, add actuators, only listen over HTTPS, improve usability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bart-vmware bart-vmware bart-vmware left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@TimHess @bart-vmware