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

Add certificate support for Azure provider #379

Closed
wants to merge 3 commits into from
Closed

Add certificate support for Azure provider #379

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f61ecf1
Add support for client certificate instead of client secret (see http…
uncaught Sep 9, 2022
0d6f9db
Add Azure client certificate configuration options to README.md.
uncaught Oct 1, 2022
c67ac09
Updated changes with php-cs-fixer and update_readme
uncaught Oct 8, 2022
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
4 changes: 4 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,11 @@ knpu_oauth2_client:
type: azure
# add and set these environment variables in your .env files
client_id: '%env(OAUTH_AZURE_CLIENT_ID)%'
# client_secret is optional if you use a client certificate
client_secret: '%env(OAUTH_AZURE_CLIENT_SECRET)%'
# Using a client certificate requires thenetworg/oauth2-azure > 2.1.1:
# client_certificate_private_key: '%env(OAUTH_AZURE_CLIENT_CERTIFICATE_PRIVATE_KEY)%'
Copy link

Choose a reason for hiding this comment

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

for custom parameters I would suggest implement some client_extra_params, because every provider could have some custom implementation.

Copy link

Choose a reason for hiding this comment

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

Or maybe some option to pass a client configured!

Copy link
Contributor Author

@uncaught uncaught Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why is this different from all the other provider specific parameters like apple.key_file_id etc.?

Copy link

Choose a reason for hiding this comment

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

IMO we we are just growing and growing this config!

for example

provider A, B, C, D ...: need the config

  • X1
  • X2
  • X3

provider W: need the config

  • X4
  • X5
  • X6

provider Y: need the config

  • X7
  • X8
  • X9

most of the providers needs only the configs X1,2,3 But because few providers I need to add more and more config,
I don't see it as a good approach!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but what exactly is the problem with that? The list of all the configs are only globally visible in the README.md, which simply lists all the providers with all their configs.

If you don't want to list all providers there anymore, then you could move the documentation to provider specific markdown files.

But the actual config parameters in the code were added in the provider specific configurator class. I don't see where this approach could be called growing out of hand.

The base configuration already only contains 4-5 options and leaves the rest to the provider specific configuration. If you want to change that, maybe by moving these 4-5 options to each provider individually and have no base config anymore, then that would be fine with me, but that would be a different PR then, setting a new standard for implementing providers.

# client_certificate_thumbprint: '%env(OAUTH_AZURE_CLIENT_CERTIFICATE_THUMBPRINT)%'
# a route name you'll create
redirect_route: connect_azure_check
redirect_params: {}
Expand Down
41 changes: 40 additions & 1 deletion src/DependencyInjection/Providers/AzureProviderConfigurator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,32 @@

use Symfony\Component\Config\Definition\Builder\NodeBuilder;

class AzureProviderConfigurator implements ProviderConfiguratorInterface
class AzureProviderConfigurator implements ProviderConfiguratorInterface, ProviderWithoutClientSecretConfiguratorInterface
uncaught marked this conversation as resolved.
Show resolved Hide resolved
{

public function needsClientSecret(): bool
{
//We define the `client_secret`-node ourselves to make it optional with certificate
return false;
}

public function buildConfiguration(NodeBuilder $node)
{
$node
->scalarNode('client_secret')
->info('The shared client secret')
->defaultValue('')
->end()
->scalarNode('client_certificate_private_key')
->example('-----BEGIN RSA PRIVATE KEY-----\nMIIEog...G82ARGuI=\n-----END RSA PRIVATE KEY-----')
->info('The contents of the client certificate private key')
->defaultValue('')
->end()
->scalarNode('client_certificate_thumbprint')
->example('B4A94A83092455AC4D3AC827F02B61646EAAC43D')
->info('The hexadecimal thumbprint of the client certificate')
->defaultValue('')
->end()
->scalarNode('url_login')
->info('Domain to build login URL')
->example("url_login: 'https://login.microsoftonline.com/'")
Expand Down Expand Up @@ -65,6 +86,22 @@ public function buildConfiguration(NodeBuilder $node)
->info('The endpoint version to run against')
->defaultValue('1.0')
->end();

//Validate that either client_secret or client_certificate_private_key is set:
$node
->end()
->validate()
->ifTrue(function($v) {
return empty($v['client_secret']) && empty($v['client_certificate_private_key']);
})
->thenInvalid('You have to define either client_secret or client_certificate_private_key')
->end()
->validate()
->ifTrue(function($v) {
return !empty($v['client_certificate_private_key']) && empty($v['client_certificate_thumbprint']);
})
->thenInvalid('You have to define the client_certificate_thumbprint when using a certificate')
->end();
}

public function getProviderClass(array $config)
Expand All @@ -75,6 +112,8 @@ public function getProviderClass(array $config)
public function getProviderOptions(array $config)
{
return [
'clientCertificatePrivateKey' => $config['client_certificate_private_key'],
'clientCertificateThumbprint' => $config['client_certificate_thumbprint'],
'clientId' => $config['client_id'],
'clientSecret' => $config['client_secret'],
'urlLogin' => $config['url_login'],
Expand Down