You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "bruno-roustant (via GitHub)" <gi...@apache.org> on 2023/06/06 15:57:44 UTC
[GitHub] [solr-sandbox] bruno-roustant opened a new pull request, #60: Encryption KeySupplier supports params to get the key cookie.
bruno-roustant opened a new pull request, #60:
URL: https://github.com/apache/solr-sandbox/pull/60
Additional parameters can be provided to the EncryptionRequestHandler in the SolrQueryRequest. The handler can be extended to send the relevant parameters to the KeySupplier when retrieving the key cookie (generic key-value pairs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] dsmiley commented on a diff in pull request #60: Encryption KeySupplier supports params to get the key cookie.
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #60:
URL: https://github.com/apache/solr-sandbox/pull/60#discussion_r1220121924
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java:
##########
@@ -238,36 +245,39 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
private void commitEmptyIndexForEncryption(@Nullable String keyId,
SegmentInfos segmentInfos,
- SolrQueryRequest req) throws IOException {
+ SolrQueryRequest req,
+ SolrQueryResponse rsp) throws IOException {
// Commit no change, with the new active key id in the commit user data.
log.debug("commit on empty index for keyId={}", keyId);
CommitUpdateCommand commitCmd = new CommitUpdateCommand(req, false);
commitCmd.commitData = new HashMap<>(segmentInfos.getUserData());
commitCmd.commitData.remove(COMMIT_ENCRYPTION_PENDING);
- setNewActiveKeyIdInCommit(keyId, commitCmd, req);
+ setNewActiveKeyIdInCommit(keyId, commitCmd, req, rsp);
assert !commitCmd.commitData.isEmpty();
req.getCore().getUpdateHandler().commit(commitCmd);
}
- private void setNewActiveKeyIdInCommit(String keyId, CommitUpdateCommand commitCmd, SolrQueryRequest req)
- throws IOException {
+ private void setNewActiveKeyIdInCommit(String keyId,
Review Comment:
_(I'm just rambling; no action asked)_ note that an UpdateCommand contains the SolrQueryRequest, albeit not the SolrQueryResponse. So it's kind of redundant to supply req. I wish the SolrQueryResponse instance, in Solr, was retrievable from the SolrQueryRequest as it's super common to want both, and they are effectively paired.
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionUtil.java:
##########
@@ -164,8 +196,39 @@ public static void clearOldInactiveKeyIdsFromCommit(Map<String, String> commitUs
inactiveKeyRefs.sort(Comparator.naturalOrder());
for (Integer keyRef : inactiveKeyRefs.subList(0, inactiveKeyRefs.size() - INACTIVE_KEY_IDS_TO_KEEP)) {
commitUserData.remove(COMMIT_KEY_ID + keyRef);
- commitUserData.remove(COMMIT_KEY_COOKIE + keyRef);
+ String commitKeyCookie = COMMIT_KEY_COOKIE + keyRef;
+ String cookieSizeAsString = commitUserData.remove(commitKeyCookie);
+ if (cookieSizeAsString != null) {
+ int cookieSize = Integer.parseInt(cookieSizeAsString);
+ for (int i = 0; i < cookieSize; i++) {
+ String entryPrefix = commitKeyCookie + '.' + i + '.';
+ commitUserData.remove(entryPrefix + 'k');
+ commitUserData.remove(entryPrefix + 'v');
+ }
+ }
}
}
}
+
+ /**
+ * Key cookie key-value pairs optionally associated to a key id in the commit user data.
+ */
+ public static class KeyCookies {
+
+ private static final KeyCookies EMPTY = new KeyCookies(Map.of());
+
+ private final Map<String, Map<String, String>> cookies;
Review Comment:
cookiesByKey would be clearer
##########
encryption/src/main/java/org/apache/solr/encryption/KeySupplier.java:
##########
@@ -17,59 +17,64 @@
package org.apache.solr.encryption;
import org.apache.lucene.index.IndexFileNames;
+import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.NamedList;
+import javax.annotation.Nullable;
import java.io.IOException;
+import java.util.Map;
import java.util.function.Function;
/**
- * Manages encryption keys and defines which index files to encrypt.
- * Supplies the encryption key secrets corresponding to provided key ids.
+ * Provides encryption key secrets corresponding to provided key ids and defines which index files to encrypt.
*/
-public interface KeyManager {
+public interface KeySupplier {
/**
- * Indicates whether the provided file is encryptable based on its name.
+ * Indicates whether the provided file should be encrypted based on its name.
* <p/>
* Segments files ({@link IndexFileNames#SEGMENTS} or {@link IndexFileNames#PENDING_SEGMENTS}) are never
* passed as parameter because they are filtered before calling this method (they must not be encrypted).
*/
- boolean isEncryptable(String fileName);
+ boolean shouldEncrypt(String fileName);
/**
- * Gets the cookie corresponding to a given key.
- * The cookie is an additional binary data to provide to get the key secret.
+ * Gets the optional cookie corresponding to a given key.
+ * The cookie is a set of key-value pairs to provide to {@link #getKeySecret}.
*
+ * @param params optional parameters in addition to the key id; or null if none.
+ * @return the key-value pairs; or null if none.
* @throws java.util.NoSuchElementException if the key is unknown.
*/
- byte[] getKeyCookie(String keyId) throws IOException;
+ @Nullable
+ Map<String, String> getKeyCookie(String keyId, Map<String, String> params) throws IOException;
/**
* Gets the encryption key secret corresponding to the provided key id.
+ * Typically, this {@link KeySupplier} holds a cache of key secrets, and may load the key secret if it is
+ * not in the cache or after expiration. In this case, this method may need to get additional data from
+ * the key cookie to load the key secret.
*
* @param keyId Key id which identifies uniquely the encryption key.
- * @param keyRef Key internal reference number to provide to the cookie supplier to retrieve the
- * corresponding cookie, if any.
- * @param cookieSupplier Takes the key reference number as input and supplies an additional binary data
- * cookie required to get the key secret. This supplier may not be called if the
- * key secret is in the transient memory cache. It may return null if there are no
- * cookies.
+ * @param cookieSupplier Takes the key id as input and supplies the optional key cookie key-value pairs
+ * that may be needed to retrieve the key secret. It may return null if there are
+ * no cookies for the key.
* @return The key secret bytes. It must be either 16, 24 or 32 bytes long. The caller is not permitted
* to modify its content. Returns null if the key is known but has no secret bytes, in this case the data
* is not encrypted.
* @throws java.util.NoSuchElementException if the key is unknown.
*/
- byte[] getKeySecret(String keyId, String keyRef, Function<String, byte[]> cookieSupplier) throws IOException;
+ byte[] getKeySecret(String keyId, Function<String, Map<String, String>> cookieSupplier) throws IOException;
/**
- * Supplies the {@link KeyManager}.
+ * Creates {@link KeySupplier}.
*/
- interface Supplier {
+ interface Factory {
- /** This supplier may be configured with parameters defined in solrconfig.xml. */
- void init(NamedList<?> args);
Review Comment:
solrconfig things are NamedList; why use Params? Should have implemented NamedListInitializedPlugin
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java:
##########
@@ -238,36 +245,39 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
private void commitEmptyIndexForEncryption(@Nullable String keyId,
SegmentInfos segmentInfos,
- SolrQueryRequest req) throws IOException {
+ SolrQueryRequest req,
+ SolrQueryResponse rsp) throws IOException {
// Commit no change, with the new active key id in the commit user data.
log.debug("commit on empty index for keyId={}", keyId);
CommitUpdateCommand commitCmd = new CommitUpdateCommand(req, false);
commitCmd.commitData = new HashMap<>(segmentInfos.getUserData());
commitCmd.commitData.remove(COMMIT_ENCRYPTION_PENDING);
- setNewActiveKeyIdInCommit(keyId, commitCmd, req);
+ setNewActiveKeyIdInCommit(keyId, commitCmd, req, rsp);
assert !commitCmd.commitData.isEmpty();
req.getCore().getUpdateHandler().commit(commitCmd);
}
- private void setNewActiveKeyIdInCommit(String keyId, CommitUpdateCommand commitCmd, SolrQueryRequest req)
- throws IOException {
+ private void setNewActiveKeyIdInCommit(String keyId,
+ CommitUpdateCommand commitCmd,
+ SolrQueryRequest req,
+ SolrQueryResponse rsp) throws IOException {
if (keyId == null) {
removeActiveKeyRefFromCommit(commitCmd.commitData);
ensureNonEmptyCommitDataForEmptyCommit(commitCmd.commitData);
} else {
- byte[] keyCookie = getKeyManager(req).getKeyCookie(keyId);
+ KeySupplier keySupplier = ((EncryptionDirectoryFactory) req.getCore().getDirectoryFactory()).getKeySupplier();
+ Map<String, String> keyCookie = keySupplier.getKeyCookie(keyId, buildGetCookieParams(req, rsp));
EncryptionUtil.setNewActiveKeyIdInCommit(keyId, keyCookie, commitCmd.commitData);
}
}
- private KeyManager getKeyManager(SolrQueryRequest req) {
- try {
- return ((EncryptionDirectoryFactory) req.getCore().getDirectoryFactory()).getKeyManager();
- } catch (ClassCastException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE,
- "DirectoryFactory class must be set to " + EncryptionDirectoryFactory.class.getName() + " to use " + getClass().getSimpleName(),
- e);
- }
+ /**
+ * Can be extended to build cookie params based on the request.
Review Comment:
Normally methods start by saying what it does, and not wether or not it can be extended (the "protected" keyword basically speaks for itself). But okay. Is the map returned to be considered immutable?
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionUtil.java:
##########
@@ -142,6 +144,36 @@ public static boolean hasKeyIdInCommit(Map<String, String> commitUserData) {
return false;
}
+ /**
+ * Gets the cookies (key-value pairs) for all the key ids, from the provided commit user data.
+ *
+ * @return the cookies for all key ids.
+ */
+ public static KeyCookies getKeyCookiesFromCommit(Map<String, String> commitUserData) {
+ Map<String, Map<String, String>> keyCookies = null;
+ for (Map.Entry<String, String> dataEntry : commitUserData.entrySet()) {
+ if (dataEntry.getKey().startsWith(COMMIT_KEY_ID)) {
+ String keyId = dataEntry.getValue();
+ String keyRef = dataEntry.getKey().substring(COMMIT_KEY_ID.length());
+ String commitKeyCookie = COMMIT_KEY_COOKIE + keyRef;
+ String cookieSizeAsString = commitUserData.get(commitKeyCookie);
+ if (cookieSizeAsString != null) {
+ int cookieSize = Integer.parseInt(cookieSizeAsString);
+ Map<String, String> cookie = new HashMap<>((int) (cookieSize / 0.75f) + 1);
Review Comment:
Honestly, I never know if that argument is valid or not and I suspect other readers won't either. :-) I suggest not over-engineering this right now but maybe add a convenience method for hashMap sizing -- not sure if we got to that in Solr yet. I remember a discussion with @risdenk about it with you.
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionUtil.java:
##########
@@ -142,6 +144,36 @@ public static boolean hasKeyIdInCommit(Map<String, String> commitUserData) {
return false;
}
+ /**
+ * Gets the cookies (key-value pairs) for all the key ids, from the provided commit user data.
+ *
+ * @return the cookies for all key ids.
+ */
+ public static KeyCookies getKeyCookiesFromCommit(Map<String, String> commitUserData) {
Review Comment:
This is awkward. Should we use JSON to embed data? Just a thought; needn't change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] bruno-roustant commented on a diff in pull request #60: Encryption KeySupplier supports params to get the key cookie.
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #60:
URL: https://github.com/apache/solr-sandbox/pull/60#discussion_r1221223230
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionUtil.java:
##########
@@ -142,6 +144,36 @@ public static boolean hasKeyIdInCommit(Map<String, String> commitUserData) {
return false;
}
+ /**
+ * Gets the cookies (key-value pairs) for all the key ids, from the provided commit user data.
+ *
+ * @return the cookies for all key ids.
+ */
+ public static KeyCookies getKeyCookiesFromCommit(Map<String, String> commitUserData) {
+ Map<String, Map<String, String>> keyCookies = null;
+ for (Map.Entry<String, String> dataEntry : commitUserData.entrySet()) {
+ if (dataEntry.getKey().startsWith(COMMIT_KEY_ID)) {
+ String keyId = dataEntry.getValue();
+ String keyRef = dataEntry.getKey().substring(COMMIT_KEY_ID.length());
+ String commitKeyCookie = COMMIT_KEY_COOKIE + keyRef;
+ String cookieSizeAsString = commitUserData.get(commitKeyCookie);
+ if (cookieSizeAsString != null) {
+ int cookieSize = Integer.parseInt(cookieSizeAsString);
+ Map<String, String> cookie = new HashMap<>((int) (cookieSize / 0.75f) + 1);
Review Comment:
I'll bump the dependency version for the encryption module so I can use CollectionUti.newHashMap() (which has the same code).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] bruno-roustant merged pull request #60: Encryption KeySupplier supports params to get the key cookie.
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant merged PR #60:
URL: https://github.com/apache/solr-sandbox/pull/60
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] bruno-roustant commented on a diff in pull request #60: Encryption KeySupplier supports params to get the key cookie.
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #60:
URL: https://github.com/apache/solr-sandbox/pull/60#discussion_r1221577872
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionUtil.java:
##########
@@ -142,6 +144,36 @@ public static boolean hasKeyIdInCommit(Map<String, String> commitUserData) {
return false;
}
+ /**
+ * Gets the cookies (key-value pairs) for all the key ids, from the provided commit user data.
+ *
+ * @return the cookies for all key ids.
+ */
+ public static KeyCookies getKeyCookiesFromCommit(Map<String, String> commitUserData) {
Review Comment:
Good point. I'll try to use JSON, this should simplify the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] bruno-roustant commented on a diff in pull request #60: Encryption KeySupplier supports params to get the key cookie.
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #60:
URL: https://github.com/apache/solr-sandbox/pull/60#discussion_r1221223230
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionUtil.java:
##########
@@ -142,6 +144,36 @@ public static boolean hasKeyIdInCommit(Map<String, String> commitUserData) {
return false;
}
+ /**
+ * Gets the cookies (key-value pairs) for all the key ids, from the provided commit user data.
+ *
+ * @return the cookies for all key ids.
+ */
+ public static KeyCookies getKeyCookiesFromCommit(Map<String, String> commitUserData) {
+ Map<String, Map<String, String>> keyCookies = null;
+ for (Map.Entry<String, String> dataEntry : commitUserData.entrySet()) {
+ if (dataEntry.getKey().startsWith(COMMIT_KEY_ID)) {
+ String keyId = dataEntry.getValue();
+ String keyRef = dataEntry.getKey().substring(COMMIT_KEY_ID.length());
+ String commitKeyCookie = COMMIT_KEY_COOKIE + keyRef;
+ String cookieSizeAsString = commitUserData.get(commitKeyCookie);
+ if (cookieSizeAsString != null) {
+ int cookieSize = Integer.parseInt(cookieSizeAsString);
+ Map<String, String> cookie = new HashMap<>((int) (cookieSize / 0.75f) + 1);
Review Comment:
I'll bump the dependency version for the encryption module so I can use CollectionUtil.newHashMap() (which has the same code).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org