-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
preflight check for password_secret #21491
Open
todvora
wants to merge
10
commits into
master
Choose a base branch
from
feature/password_secret_preflight_check
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
0b1f4fc
preflight check for password_secret
todvora 77a7afc
fixed annotation
todvora 4330e51
Merge branch 'master' into feature/password_secret_preflight_check
todvora e160bbb
added changelog
todvora de61376
preflight object mapper provider
todvora ab16ef3
Merge branch 'master' into feature/password_secret_preflight_check
moesterheld 81e2553
Merge branch 'master' into feature/password_secret_preflight_check
moesterheld c25849b
code cleanup
todvora f6e6460
Update graylog2-server/src/main/java/org/graylog2/bootstrap/preflight…
todvora eba0e17
Merge branch 'master' into feature/password_secret_preflight_check
todvora File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
type = "a" | ||
message = "Add preflight check for data node and graylog server, veryfing that password_secret is configured to the same value everywhere." | ||
|
||
pulls = ["21491"] | ||
issues = ["21504"] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
53 changes: 53 additions & 0 deletions
53
data-node/src/main/java/org/graylog/datanode/bindings/PreflightObjectMapperProvider.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/* | ||
* Copyright (C) 2020 Graylog, Inc. | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the Server Side Public License, version 1, | ||
* as published by MongoDB, Inc. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* Server Side Public License for more details. | ||
* | ||
* You should have received a copy of the Server Side Public License | ||
* along with this program. If not, see | ||
* <http://www.mongodb.com/licensing/server-side-public-license>. | ||
*/ | ||
package org.graylog.datanode.bindings; | ||
|
||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import jakarta.inject.Inject; | ||
import jakarta.inject.Provider; | ||
import org.graylog.grn.GRNRegistry; | ||
import org.graylog2.jackson.InputConfigurationBeanDeserializerModifier; | ||
import org.graylog2.security.encryption.EncryptedValueService; | ||
import org.graylog2.shared.bindings.providers.ObjectMapperProvider; | ||
import org.graylog2.shared.plugins.GraylogClassLoader; | ||
|
||
import java.util.Collections; | ||
|
||
/** | ||
* This ObjectMapperProvider should be used only for preflight checks and preflight web. It's significantly limited. | ||
* For all other usages, please refer to {@link ObjectMapperProvider}. | ||
*/ | ||
public class PreflightObjectMapperProvider implements Provider<ObjectMapper> { | ||
|
||
private final ObjectMapperProvider delegate; | ||
|
||
@Inject | ||
public PreflightObjectMapperProvider(@GraylogClassLoader ClassLoader classLoader, EncryptedValueService encryptedValueService) { | ||
delegate = new ObjectMapperProvider( | ||
classLoader, | ||
Collections.emptySet(), | ||
encryptedValueService, | ||
GRNRegistry.createWithBuiltinTypes(), | ||
InputConfigurationBeanDeserializerModifier.withoutConfig() | ||
); | ||
} | ||
|
||
@Override | ||
public ObjectMapper get() { | ||
return delegate.get(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
83 changes: 83 additions & 0 deletions
83
...2-server/src/main/java/org/graylog2/bootstrap/preflight/PasswordSecretPreflightCheck.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
/* | ||
* Copyright (C) 2020 Graylog, Inc. | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the Server Side Public License, version 1, | ||
* as published by MongoDB, Inc. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* Server Side Public License for more details. | ||
* | ||
* You should have received a copy of the Server Side Public License | ||
* along with this program. If not, see | ||
* <http://www.mongodb.com/licensing/server-side-public-license>. | ||
*/ | ||
package org.graylog2.bootstrap.preflight; | ||
|
||
import jakarta.inject.Inject; | ||
import org.graylog2.plugin.cluster.ClusterConfigService; | ||
import org.graylog2.security.encryption.EncryptedValue; | ||
import org.graylog2.security.encryption.EncryptedValueService; | ||
|
||
import javax.annotation.Nonnull; | ||
import java.util.Optional; | ||
|
||
/** | ||
* This preflight check exists to validate that every node has the same password_secret value configured. | ||
* The first node in the cluster will persist a known and encoded secret in the cluster_config collection. | ||
* Every other node is then testing if it's possible to read and decode the value. If not, then the password_secret | ||
* is not matching and we'd get problems later during runtime. So it's better to stop the startup and report | ||
* an explicit and readable error. | ||
*/ | ||
public class PasswordSecretPreflightCheck implements PreflightCheck { | ||
|
||
|
||
private static final String KNOWN_VALUE = "graylog"; | ||
|
||
private final ClusterConfigService clusterConfigService; | ||
|
||
private final EncryptedValueService encryptionService; | ||
|
||
|
||
@Inject | ||
public PasswordSecretPreflightCheck(ClusterConfigService clusterConfigService, EncryptedValueService encryptionService) { | ||
this.clusterConfigService = clusterConfigService; | ||
this.encryptionService = encryptionService; | ||
} | ||
|
||
@Override | ||
public void runCheck() throws PreflightCheckException { | ||
final PreflightEncryptedSecret encryptedValue = clusterConfigService.get(PreflightEncryptedSecret.class); | ||
Optional.ofNullable(encryptedValue) | ||
.ifPresentOrElse(this::validateSecret, this::persistSecret); | ||
} | ||
|
||
private void persistSecret() { | ||
final EncryptedValue encryptedValue = encryptionService.encrypt(KNOWN_VALUE); | ||
clusterConfigService.write(new PreflightEncryptedSecret(encryptedValue)); | ||
} | ||
|
||
private void validateSecret(@Nonnull PreflightEncryptedSecret preflightEncryptedSecret) { | ||
final EncryptedValue encryptedSecret = preflightEncryptedSecret.encryptedSecret(); | ||
|
||
try { | ||
final String decrypted = encryptionService.decrypt(encryptedSecret); | ||
if (!KNOWN_VALUE.equals(decrypted)) { | ||
throwException(); | ||
} | ||
} catch (Exception e) { | ||
throwException(); | ||
} | ||
|
||
} | ||
|
||
private void throwException() { | ||
throw new PreflightCheckException(""" | ||
Invalid password_secret! | ||
Failed to decrypt values from MongoDB. This means that your password_secret has been changed or there | ||
are some nodes in your cluster that are using a different password_secret to the one configured on this node. Secrets have to be configured | ||
to the same value on every node and can't be changed afterwards."""); | ||
} | ||
} |
22 changes: 22 additions & 0 deletions
22
graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/PreflightEncryptedSecret.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/* | ||
* Copyright (C) 2020 Graylog, Inc. | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the Server Side Public License, version 1, | ||
* as published by MongoDB, Inc. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* Server Side Public License for more details. | ||
* | ||
* You should have received a copy of the Server Side Public License | ||
* along with this program. If not, see | ||
* <http://www.mongodb.com/licensing/server-side-public-license>. | ||
*/ | ||
package org.graylog2.bootstrap.preflight; | ||
|
||
import org.graylog2.security.encryption.EncryptedValue; | ||
|
||
public record PreflightEncryptedSecret(EncryptedValue encryptedSecret) { | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
72 changes: 72 additions & 0 deletions
72
...rver/src/test/java/org/graylog2/bootstrap/preflight/PasswordSecretPreflightCheckTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
/* | ||
* Copyright (C) 2020 Graylog, Inc. | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the Server Side Public License, version 1, | ||
* as published by MongoDB, Inc. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* Server Side Public License for more details. | ||
* | ||
* You should have received a copy of the Server Side Public License | ||
* along with this program. If not, see | ||
* <http://www.mongodb.com/licensing/server-side-public-license>. | ||
*/ | ||
package org.graylog2.bootstrap.preflight; | ||
|
||
|
||
import jakarta.annotation.Nonnull; | ||
import org.apache.commons.lang3.RandomStringUtils; | ||
import org.assertj.core.api.Assertions; | ||
import org.graylog.security.certutil.InMemoryClusterConfigService; | ||
import org.graylog2.plugin.cluster.ClusterConfigService; | ||
import org.graylog2.security.encryption.EncryptedValueService; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class PasswordSecretPreflightCheckTest { | ||
|
||
@Test | ||
void testEmptyDB() { | ||
final String passwordSecret = RandomStringUtils.secure().nextAlphanumeric(20); | ||
final InMemoryClusterConfigService clusterConfigService = new InMemoryClusterConfigService(); | ||
final PasswordSecretPreflightCheck preflightCheck = createCheckInstance(passwordSecret, clusterConfigService); | ||
preflightCheck.runCheck(); | ||
} | ||
|
||
@Test | ||
void testSuccessfulValidation() { | ||
final String passwordSecret = RandomStringUtils.secure().nextAlphanumeric(20); | ||
final InMemoryClusterConfigService clusterConfigService = new InMemoryClusterConfigService(); | ||
final PasswordSecretPreflightCheck preflightCheck = createCheckInstance(passwordSecret, clusterConfigService); | ||
|
||
// first check will persist the value in the DB | ||
preflightCheck.runCheck(); | ||
|
||
// second should read it from there and validate | ||
preflightCheck.runCheck(); | ||
} | ||
|
||
@Test | ||
void testFailingValidation() { | ||
final RandomStringUtils randomStringUtils = RandomStringUtils.secure(); | ||
// first check will persist the value in the DB | ||
final InMemoryClusterConfigService clusterConfigService = new InMemoryClusterConfigService(); | ||
createCheckInstance(randomStringUtils.nextAlphanumeric(20), clusterConfigService).runCheck(); | ||
|
||
Assertions.assertThatThrownBy(() -> { | ||
// now repeat the check, but with different password. Should fail | ||
createCheckInstance(randomStringUtils.nextAlphanumeric(20), clusterConfigService).runCheck(); | ||
}) | ||
.isInstanceOf(PreflightCheckException.class) | ||
.hasMessageContaining("Invalid password_secret"); | ||
|
||
} | ||
|
||
@Nonnull | ||
private static PasswordSecretPreflightCheck createCheckInstance(String passwordSecret, ClusterConfigService clusterConfigService) { | ||
final EncryptedValueService encryptionService = new EncryptedValueService(passwordSecret); | ||
return new PasswordSecretPreflightCheck(clusterConfigService, encryptionService); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, our implementation in
AESTools
doesn't guarantee that the secret is identical as it modifies the key. You can reproduce that by removing a single letter from the end of a long password secret. This will still lead to problems with the keystore password.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a problem related to this PR. For the mongodb entries encryption, this will detect any problem. Keystore reading will be a local problem that we can (and should) capture by proper error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure. I would expect the check to make sure that password_secrets are identical on all nodes, no matter if they are used for encryption or anything else. This is also what the class' javadoc implies.
With the given check, there is still a margin for error which is difficult to track down. Something that we wanted to avoid with this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could we fix that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't thought it through completely yet, but what if we don't encrypt a constant value but the secret itself? Then, even if you can decrypt it with a shorter secret, you could still make sure that they are identical