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

[ELY-2078] Add encryption support to FileSystemSecurityRealm #1676

Merged
merged 4 commits into from
Mar 18, 2022

Conversation

Ashpan
Copy link
Contributor

@Ashpan Ashpan commented Mar 1, 2022

@fjuma fjuma changed the title [ELY-2078] Add encryption and integrity support to FileSystemSecurityRealm [ELY-2078] Add encryption support to FileSystemSecurityRealm Mar 1, 2022
@Ashpan Ashpan force-pushed the ELY-2078-encryption branch 2 times, most recently from 2e611f4 to a8d221b Compare March 1, 2022 19:05
@fjuma
Copy link
Contributor

fjuma commented Mar 1, 2022

@Ashpan The fix for ELY-2249 has already been merged so that commit can be removed.

* @author <a href="mailto:araskar@redhat.com">Ashpan Raskar</a>
*/
public class FileSystemRealmUtil {
public static void createEncryptedRealmFromUnencrypted(FileSystemSecurityRealm unencryptedRealm, FileSystemSecurityRealm encryptedRealm) throws RealmUnavailableException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add javadoc for this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to add checks to make sure the params aren't null.

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to the variable descriptions, would be good to add a description of what the method does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@@ -855,7 +886,10 @@ private void writeIdentity(final XMLStreamWriter streamWriter, final LoadedIdent
String passwordString;
byte[] encoded = BasicPasswordSpecEncoding.encode(password, providers);

if (encoded != null) {
if (this.secretKey != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A very minor comment but just wanted to mention for readability that it's not necessary to use this here or below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

streamWriter.writeAttribute("name", entry.getKey());
streamWriter.writeAttribute("value", value);
streamWriter.writeAttribute("name", this.secretKey != null ? CipherUtil.encrypt(entry.getKey(), this.secretKey) : entry.getKey());
streamWriter.writeAttribute("value", this.secretKey != null ? CipherUtil.encrypt(value, this.secretKey) : value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like some extra indentation has been added here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -1012,7 +1046,7 @@ private LoadedIdentity parseIdentityContents(final XMLStreamReader streamReader,
}
List<Credential> credentials = new ArrayList<>();
do {
if (! version.getNamespace().equals(streamReader.getNamespaceURI())) {
if ( !(version.getNamespace().equals(streamReader.getNamespaceURI())) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like an extra set of brackets has been added here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

throw ElytronMessages.log.fileSystemRealmMissingAttribute("algorithm", path, streamReader.getLocation().getLineNumber(), name);
}
PasswordFactory passwordFactory = PasswordFactory.getInstance(algorithm);
byte[] passwordByte = CodePointIterator.ofChars(text.toCharArray()).base64Decode().drain();
Copy link
Contributor

Choose a reason for hiding this comment

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

passwordBytes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@Ashpan Ashpan force-pushed the ELY-2078-encryption branch 2 times, most recently from b7e984a to 4e56cda Compare March 1, 2022 20:43
@@ -233,6 +247,8 @@ public FileSystemSecurityRealm(final Path root, final NameRewriter nameRewriter,
this(root, nameRewriter, levels, true);
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to remove the extra white space in this file.

*
* @param unencryptedRealm the {@code FileSystemSecurityRealm} without any encryption applied
* @param encryptedRealm the {@code FileSystemSecurityRealm} configured with a SecretKey to encrypt identity data
* @throws RealmUnavailableException
Copy link
Contributor

Choose a reason for hiding this comment

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

@throws RealmUnavailableException if either realm is unavailable

@@ -640,7 +696,7 @@ private Void createPrivileged() throws RealmUnavailableException {
streamWriter.writeCharacters("\n");
streamWriter.writeStartElement("identity");
// Continue to write using 1.0 as not using any features added in 1.0.1
streamWriter.writeDefaultNamespace(ELYTRON_1_0);
streamWriter.writeDefaultNamespace(Version.VERSION_1_0.getNamespace());
Copy link
Contributor

Choose a reason for hiding this comment

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

When secretKey is not null, we should be writing version 1.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The schema doesn't change when using an encrypted realm. Would this still require a Namespace bump?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, are the schema changes only for the integrity portion of the changes? If so, ignore this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, was just thinking some more, the format value "enc_base64" is only allowed in the new 1.1 schema. This isn't a valid attribute for 1.0. So I think we should make the suggested changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

private Version requiredVersion(final LoadedIdentity identityToWrite) {
// As new functionality is added we will identify if we need to use a later version
// if new functionality is used then use the required schema version otherwise fallback
// to an older version.
Copy link
Contributor

Choose a reason for hiding this comment

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

If secretKey is not null, the required version should be 1.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@Ashpan Ashpan force-pushed the ELY-2078-encryption branch 2 times, most recently from 5ec1520 to b144343 Compare March 1, 2022 21:03
/**
* Set whether the identity name should be encoded for the filename in the realm.
*
* @param encoded whether identity names should be BASE32 encoded before using as filename if secretKey is not specified
Copy link
Contributor

Choose a reason for hiding this comment

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

encoded whether identity names should be BASE32 encoded before using as filename (only applies if the security realm is unencrypted)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

import java.security.Security;
import java.util.HashMap;
import java.util.Map;
import org.apache.commons.cli.AlreadySelectedException;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to not re-order imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -27,11 +31,6 @@
import org.jboss.logging.annotations.Message;
import org.jboss.logging.annotations.MessageLogger;

import java.io.FileNotFoundException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -234,8 +233,6 @@ public FileSystemSecurityRealm(final Path root, final NameRewriter nameRewriter,
this(root, nameRewriter, levels, encoded, Encoding.BASE64, StandardCharsets.UTF_8, INSTALLED_PROVIDERS, null);
}


Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to remove the white space changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

import org.wildfly.security.password.spec.Encoding;

/**
* Elytron-Tool command to convert un-encrypted FileSystemRealms into an encrypted realm with the use of a SecreKey.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/SecreKey/SecretKey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


class FileSystemEncryptRealmCommand extends Command {
static final int GENERAL_CONFIGURATION_WARNING = 1;
static final String FILE_SYSTEM_ENCRYPT_COMMAND = "filesystem-encrypt";
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to include realm in the name, e.g., filesystem-realm-encrypt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

try {
descriptor.setHashCharset(Charset.forName(hashCharsetOption));
} catch (UnsupportedCharsetException e) {
// Error
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if something went wrong when updating your commits. I think I had previously added a comment about updating the error handling in this file but I don't see those changes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I lost those changes in the rebasing, fetching those changes from the original PR

.setRoot(Paths.get(descriptor.getOutputRealmLocation(), descriptor.getFileSystemRealmName()))
.setSecretKey(key)
.setLevels(descriptor.getLevels())
.setHashEncoding(descriptor.getHashEncoding())
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the encrypted realm actually make use of the hashEncoding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch it doesn't, I'll remove that

continue;
}
CredentialStore credentialStore;
if ((descriptor.getCredentialStore() == null) == (descriptor.getSecretKeyAlias() == null)) {
Copy link
Contributor

@fjuma fjuma Mar 1, 2022

Choose a reason for hiding this comment

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

Is this suppose to be && instead of == here?

Copy link
Contributor Author

@Ashpan Ashpan Mar 1, 2022

Choose a reason for hiding this comment

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

I did the == here to check if either both are not null or both are null.
If only 1 is defined but not the other, and error is thrown: https://github.com/wildfly-security/wildfly-elytron/pull/1676/files/b1443435ea996fc76917a3bd58c7f14852c30819#diff-ebe53241afd70e1e5743d6db5b39150461c335abce6c083cafbf6aae4b4c1e2dR650

I could also check with an ^ XOR operator

.setSecretKey(key)
.build();
ModifiableRealmIdentity identity = securityRealm.getRealmIdentityForUpdate(new NamePrincipal("plainUser"));

Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a check to make sure the identity doesn't exist here before the call to create?

@Ashpan
Copy link
Contributor Author

Ashpan commented Mar 15, 2022

@fjuma @Skyllarr All comments have been addressed now


Descriptor descriptor = new Descriptor();
descriptor.setFileSystemRealmName(realmNameOption);
descriptor.setOutputRealmLocation(Paths.get(outputRealmLocationOption).toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

When outputRealmLocationOption is not set NullPointerException is thrown

$ ./bin/elytron-tool.sh filesystem-realm-encrypt -i a --debug
Exception encountered executing the command:
java.lang.NullPointerException
	at java.base/sun.nio.fs.UnixPath.normalizeAndCheck(UnixPath.java:75)
	at java.base/sun.nio.fs.UnixPath.<init>(UnixPath.java:69)
	at java.base/sun.nio.fs.UnixFileSystem.getPath(UnixFileSystem.java:279)
	at java.base/java.nio.file.Path.of(Path.java:147)
	at java.base/java.nio.file.Paths.get(Paths.java:69)
	at org.wildfly.security.elytron-tool@1.18.4.CR1-SNAPSHOT//org.wildfly.security.tool.FileSystemEncryptRealmCommand.execute(FileSystemEncryptRealmCommand.java:325)
	at org.wildfly.security.elytron-tool@1.18.4.CR1-SNAPSHOT//org.wildfly.security.tool.ElytronTool.main(ElytronTool.java:85)
	at org.jboss.modules.Module.run(Module.java:353)
	at org.jboss.modules.Module.run(Module.java:321)
	at org.jboss.modules.Main.main(Main.java:604)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to

Exception encountered executing the command:
org.apache.commons.cli.MissingArgumentException: Output Realm location not specified.
	at org.wildfly.security.tool.FileSystemEncryptRealmCommand.execute(FileSystemEncryptRealmCommand.java:326)
	at org.wildfly.security.tool.ElytronTool.main(ElytronTool.java:85)

Descriptor descriptor = new Descriptor();
descriptor.setFileSystemRealmName(realmNameOption);
descriptor.setOutputRealmLocation(Paths.get(outputRealmLocationOption).toString());
descriptor.setInputRealmLocation(Paths.get(inputRealmLocationOption).toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

The same as for the previous line.

Copy link
Contributor Author

@Ashpan Ashpan Mar 16, 2022

Choose a reason for hiding this comment

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

Changed to

Exception encountered executing the command:
org.apache.commons.cli.MissingArgumentException: Input Realm location not specified.
	at org.wildfly.security.tool.FileSystemEncryptRealmCommand.execute(FileSystemEncryptRealmCommand.java:331)
	at org.wildfly.security.tool.ElytronTool.main(ElytronTool.java:85)

);

if (!createScriptCheck.equals("y") && !createScriptCheck.equals("yes")) {
Files.write(Paths.get(String.format("%s/%s.sh", outputRealmLocation, fileSystemRealmName)), scriptLines, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING);
Copy link
Contributor

Choose a reason for hiding this comment

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

When the outputRealmLocation directory does not exist, this results into NoSuchFileException:

$ ./bin/elytron-tool.sh filesystem-realm-encrypt -i fs-realm-users-plain -o fs-realm-users-plain-enc --debug
Creating encrypted realm for: fs-realm-users-plain
Exception encountered executing the command:
java.nio.file.NoSuchFileException: fs-realm-users-plain-enc/encrypted-filesystem-realm.sh
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:116)
	at java.base/sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:219)
	at java.base/java.nio.file.spi.FileSystemProvider.newOutputStream(FileSystemProvider.java:478)
	at java.base/java.nio.file.Files.newOutputStream(Files.java:220)
	at java.base/java.nio.file.Files.write(Files.java:3490)
	at java.base/java.nio.file.Files.write(Files.java:3541)
	at org.wildfly.security.elytron-tool@1.18.4.CR1-SNAPSHOT//org.wildfly.security.tool.FileSystemEncryptRealmCommand.createWildFlyScript(FileSystemEncryptRealmCommand.java:753)
	at org.wildfly.security.elytron-tool@1.18.4.CR1-SNAPSHOT//org.wildfly.security.tool.FileSystemEncryptRealmCommand.execute(FileSystemEncryptRealmCommand.java:388)
	at org.wildfly.security.elytron-tool@1.18.4.CR1-SNAPSHOT//org.wildfly.security.tool.ElytronTool.main(ElytronTool.java:85)
	at org.jboss.modules.Module.run(Module.java:353)
	at org.jboss.modules.Module.run(Module.java:321)
	at org.jboss.modules.Main.main(Main.java:604)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

);

if (!createScriptCheck.equals("y") && !createScriptCheck.equals("yes")) {
Files.write(Paths.get(String.format("%s/%s.sh", outputRealmLocation, fileSystemRealmName)), scriptLines, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why .sh? It's not a shell script. Something like .cli would be more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using the same implementation used in the FileSystemRealmCommand.java.
I can change it to .cli instead

} else {
descriptor.setSecretKeyAlias("key");
}
descriptors.add(descriptor);
Copy link
Contributor

Choose a reason for hiding this comment

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

findMissingRequiredValuesAndSetValues is not applied in this case, hence it's not clear what is missing. For example the following ends with no identities converted (silently skipped by the initial check in createFileSystemRealm?)

$ ./bin/elytron-tool.sh filesystem-realm-encrypt -i ./standalone/configuration/fs-realm-users-plain -o ./standalone/configuration/fs-realm-users-plain-enc --debug
Creating encrypted realm for: ./standalone/configuration/fs-realm-users-plain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a warning message to indicate something is missing. Although now it should flag it before entering that method

https://github.com/wildfly-security/wildfly-elytron/pull/1676/files#diff-ebe53241afd70e1e5743d6db5b39150461c335abce6c083cafbf6aae4b4c1e2dR664-R666

@Ashpan Ashpan force-pushed the ELY-2078-encryption branch 2 times, most recently from 46a19d5 to 7885747 Compare March 16, 2022 16:16
@Message(id = NONE, value = "Bulk conversion with options listed in description file. Optional options have default values, required options do not. (Action) %n" +
"The options realm-name, hash-encoding, levels, secret-key, and populate are optional. %n" +
"The default values of realm-name, hash-charset, hash-encoding, levels, and create are encrypted-filesystem-realm, BASE64, UTF-8, 2, and true respectively. %n" +
"Values are required for the following options: input-location, output-location, and credential-store. %n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to move the information about required values to the beginning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved

@@ -422,6 +467,25 @@
"Blocks of options must be separated by a blank line.")
String cmdFileSystemRealmBulkConvertDesc();

@Message(id = NONE, value = "Bulk conversion with options listed in description file. Optional options have default values, required options do not. (Action) %n" +
"The options realm-name, hash-encoding, levels, secret-key, and populate are optional. %n" +
"The default values of realm-name, hash-charset, hash-encoding, levels, and create are encrypted-filesystem-realm, BASE64, UTF-8, 2, and true respectively. %n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

  • hash-charset and UTF-8 should be removed from the description here
  • secret-key and its default value should be added here
  • the previous line mentions "populate" but this line mentions "create", this needs to be sorted out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

descriptor.setSecretKeyAlias("key");
}
descriptors.add(descriptor);
} else if (realmNameOption != null | inputRealmLocationOption != null |
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this else if needs to cover all the options right? I think some are missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we still missing options like hashEncoding, levels, etc.? IIUC, we get here if the bulk option is being used so any specified option should result in an error, right?

*/
private void createFileSystemRealm() throws Exception {
for (Descriptor descriptor : descriptors) {
System.out.println("Creating encrypted realm for: " + descriptor.getInputRealmLocation());
Copy link
Contributor

Choose a reason for hiding this comment

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

This message should be in the messages file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

}
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to test the case where one or more required attributes is missing both in bulk mode and regular mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests for both, testSingleUserMissingParam, testBulkMissingParam

@Ashpan Ashpan force-pushed the ELY-2078-encryption branch 3 times, most recently from a4c9cc0 to e284dbd Compare March 16, 2022 20:40
private static final class Descriptor {
private String inputRealmLocation;
private String outputRealmLocation;
private String FileSystemRealmName;
Copy link
Contributor

Choose a reason for hiding this comment

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

should be fileSystemRealmName

descriptor.setSecretKeyAlias("key");
}
descriptors.add(descriptor);
} else if (realmNameOption != null | inputRealmLocationOption != null |
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we still missing options like hashEncoding, levels, etc.? IIUC, we get here if the bulk option is being used so any specified option should result in an error, right?

…alm.

This new schema drops all elements and types specific to authentication
client.

Additionally prepare for support for new items to be added to this
namespace.
checkDescriptorFields(descriptor);
} else if (inputRealmLocationOption != null | outputRealmLocationOption != null |
realmNameOption != null | credentialStoreOption != null | createCredentialStore != null |
secretKeyAliasOption != null | hashEncodingOption != null | levelsOption != null | populateOption != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think encodedOption is the last one needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@fjuma
Copy link
Contributor

fjuma commented Mar 17, 2022

@Ashpan Just one last small thing. The fourth commit message refers to logging specifically but there are a lot of other changes in that commit too so would be good to reword it to make it more general.

@Skyllarr
Copy link
Contributor

Skyllarr commented Mar 18, 2022

@Ashpan Just one last small thing. The fourth commit message refers to logging specifically but there are a lot of other changes in that commit too so would be good to reword it to make it more general.

@Ashpan There are also some tool's command changes in that logging commit which could be moved to a previous commit regarding elytron tool. But that is just minor it is ok to merge like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants