[MGNLSSO-265] Possibility to implement custom SsoConfig Created: 18/Apr/23  Updated: 20/Oct/23  Resolved: 13/Jun/23

Status: Closed
Project: Single Sign On
Component/s: None
Affects Version/s: None
Fix Version/s: saas, 3.1.5

Type: Improvement Priority: Major
Reporter: Björn Eschle Assignee: Evzen Fochr
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: 2d 4.5h
Original Estimate: Not Specified

Attachments: Java Source File GardenaSsoConfig.java    
Issue Links:
Problem/Incident
causes MGNLSSO-316 Invalidating/reloading SSO Yaml Confi... Closed
Relates
relates to MGNLSSO-184 Property Expansion in magnolia-sso/co... Closed
relates to DOCU-2728 Update SSO docs re: passing secrets a... Closed
relates to MGNLSSO-291 Subclassing Pac4jConfigProvider from ... Closed
relates to MGNLSSO-292 Remove Microprofile config related in... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[X]* Doc/release notes changes? Comment present?
[X]* Downstream builds green?
[X]* Solution information and context easily available?
[X]* Tests
[X]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Release notes required:
Yes
Date of First Response:
Epic Link: SSO maintenance
Team: AdminX
Work Started:
Approved:
Yes

 Description   

The default ssoConfig is defined in the config yaml. This has 2 main issues:

  1. unencrypted secrets (event with your suggested env variables on build process solution, the war file contains the secret in plain text).
  2. Environment specific client configuration

 

The possibility to define a custom SsoConfig implementation would enable us to load the config from wherever we like (e.g. magnolia properties, jcr, ...)

Currently this can only be achieved by a hack:

import info.magnolia.config.source.yaml.YamlReader;
import info.magnolia.map2bean.Map2BeanTransformer;
import info.magnolia.module.ModuleRegistry;
import info.magnolia.resourceloader.ResourceOrigin;
import info.magnolia.sso.config.SsoConfig;
import info.magnolia.sso.config.SsoConfigYamlBridge;

import javax.inject.Inject;

public class CustomSsoConfigBridge extends SsoConfigYamlBridge {
    private final CustomSsoConfig ssoConfig;

    @Inject
    public GardenaSsoConfigBridge(
          final ModuleRegistry moduleRegistry,
          final ResourceOrigin resourceOrigin,
          final YamlReader yamlReader,
          final Map2BeanTransformer map2Bean,
          final CustomSsoConfig ssoConfig) {
       super(moduleRegistry, resourceOrigin, yamlReader, map2Bean);
       this.ssoConfig = ssoConfig;
    }

    @Override
    public SsoConfig get() {
       return ssoConfig;
    }
} 
import info.magnolia.sso.config.SsoConfig;

public class CustomSsoConfig implements SsoConfig {
    ...
} 

Binding in module.xml

<component>
    <type>info.magnolia.sso.config.SsoConfigYamlBridge</type>
    <implementation>...CustomSsoConfigBridge</implementation>
</component>

Binding the SsoConfig directly would be the cleaner solution and less dependent on the further development of this module (breaking change in case the Bridge ever gets removed/changed).



 Comments   
Comment by Mikaël Geljić [ 24/May/23 ]

Hi beschle,

On secrets in plain-text first:

  • in SSO 2.x, config was provided as a YAML decorator. The !env directive was not supported there. The docs mentioned a workaround to use a template file, and post-process it with the envsubst command, resulting in secrets stored in plain text, as you noted.
  • in SSO 3.x (since 3.0.1 actually), we now load config through a specific "yaml bridge", and we do support the !env directive there. So there shouldn't be any secrets in plain text anymore. We just realized that we forgot to update documentation for this. Let me file a ticket.

The other part about extending SsoConfig: if I understand your use case correctly, there is a version check (if magnolia core = 6.2) that forces to use the YamlBridge, thus ignoring your CustomSsoConfig class. We could probably implement a "config selection logic" with fallbacks for this, but first, do you still need your custom config in light of the above?

Hope that helps!
-Mika

Comment by Björn Eschle [ 25/May/23 ]

Hey @Mika,

thanks for your feedback. You mean this feature: https://docs.magnolia-cms.com/product-docs/6.2/Administration/Architecture/Configuration-management.html#_environment_variables?

Your docu (https://docs.magnolia-cms.com/magnolia-sso/3.1.3/guides/hiding-the-client-credentials-from-the-configuration-file.html) states:

Magnolia does support environment variables in YAML definitions, but, unfortunately, only for YAML definitions, not for YAML decorators, as is the case here.

However, even with env variables supported, I think allowing the binding of custom SsoConfigs will improve flexibility a lot and should not be a big effort/change on your side (I've attached a sample implementation - of gardena - so that you can get a glance on how we are currently using it).

The issue with the bridge is not with magnolia versioning, but that we are (miss-)using it to bind our custom SsoConfig. This is working well right now, but if you ever decide to change/or remove the bridge, we'll have to adapt.

One solution could be to inject a SsoConfigProvider instead of the SsoConfigYamlBridge in Pac4jConfigProvider and bind said provider to the DefaultSsoConfigProvider by default. This way we could simply change the binding to a custom SsoConfigProvider:

import info.magnolia.cms.beans.config.ServerConfiguration;
import info.magnolia.resourceloader.ResourceOrigin;
import info.magnolia.sso.config.SsoConfig;
import info.magnolia.sso.config.SsoConfigYamlBridge;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.inject.Inject;
import java.util.HashSet;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Supplier;

import static info.magnolia.resourceloader.ResourceOriginChange.Type.ADDED;
import static info.magnolia.resourceloader.ResourceOriginChange.Type.MODIFIED;

class Pac4jConfigProvider {
    private final ServerConfiguration serverConfiguration;
    private final SsoConfigProvider ssoConfigProvider;
    
    @Inject
    public Pac4jConfigProvider(
          final ServerConfiguration serverConfiguration,
          final SsoConfigProvider ssoConfigProvider
    ) {
       this.serverConfiguration = serverConfiguration;
       this.ssoConfigProvider = ssoConfigProvider;
       ssoConfigProvider.registerSsoConfigChangedListener(ssoConfig -> 
          loadPac4jConfig()
       );
    }

    SsoConfig getSsoConfig() {
       return ssoConfigProvider.get();
    }
    
    ...
}

interface SsoConfigProvider {
    SsoConfig get();
    void registerSsoConfigChangedListener(Consumer<SsoConfig> listener);
    void unregisterSsoConfigChangedListener(Consumer<SsoConfig> listener);
}

class DefaultSsoConfigProvider implements SsoConfigProvider {
    private static final Logger log = LoggerFactory.getLogger(DefaultSsoConfigProvider.class);
    private final Set<Consumer<SsoConfig>> listeners = new HashSet<>();
    private final Supplier<SsoConfig> ssoConfigSupplier;
    private SsoConfig ssoConfig;

    public DefaultSsoConfigProvider(
          final SsoConfig ssoMpConfig,
          final SsoConfigYamlBridge yamlBridge,
          final ResourceOrigin resourceOrigin
    ) {
       // if we're on 6.2, disregard the injected SSO MicroProfile config, and load if from yaml/m2b instead
       this.ssoConfigSupplier = yamlBridge.shouldLoadSsoConfigByYamlReader() ? yamlBridge : () -> ssoMpConfig;
       if (yamlBridge.shouldLoadSsoConfigByYamlReader()) {
          log.info("Monitoring SSO YAML configuration changes on {}", SsoConfigYamlBridge.SSO_YAML_CONFIG_PATH);
          resourceOrigin.registerResourceChangeHandler(change -> {
             String relatedResourcePath = change.getRelatedResourcePath();
             boolean isConfigFileModifiedOrAdded = MODIFIED.equals(change.getType()) || ADDED.equals(change.getType());
             if (isConfigFileModifiedOrAdded && relatedResourcePath.matches(SsoConfigYamlBridge.SSO_YAML_CONFIG_PATH)) {
                log.info("Receive {} change on file {}, reload SSO config and Pac4j configuration...", change.getType(), relatedResourcePath);
                ssoConfig = ssoConfigSupplier.get();
                notifyListeners();
             }
          });
       }
    }

    @Override
    public SsoConfig get() {
       if (ssoConfig == null) {
          ssoConfig = ssoConfigSupplier.get();
       }
       return ssoConfig;
    }

    private void notifyListeners() {
       listeners.forEach(listener -> listener.accept(get()));
    }
    
    @Override
    public void registerSsoConfigChangedListener(Consumer<SsoConfig> listener) {
       listeners.add(listener);
    }

    @Override
    public void unregisterSsoConfigChangedListener(Consumer<SsoConfig> listener) {
       listeners.remove(listener);
    }
} 
Comment by Mikaël Geljić [ 02/Jun/23 ]

Yes, understood,

We're considering a similar refactoring to ensure SsoConfig binding can be overridden without having to care about the YamlBridge (nor MicroProfile config moving forward).

Comment by Nguyen Phung Chi [ 12/Jun/23 ]

Hey guys, 

FYI, the documentation has been updated: https://docs.magnolia-cms.com/magnolia-sso/3.1.4/guides/hiding-the-client-credentials-from-the-configuration-file.html

Also the ticket is in final stage, will be released in SSO v3.1.5.

Thanks for your contributions mgeljic, beschle 

Comment by Björn Eschle [ 14/Jun/23 ]

Thanks for the fast implementation! Works like a charm

Generated at Mon Feb 12 10:52:37 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.