Add registrar id header to proxy requests

[ptkach]

Tested in crash, works as expected

---

This change is 

[weiminyu]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 164 at r1 (raw file):

  }

  private Optional<String> extractRegistrarIdFromSessionInfo(String sessionInfo) {

Add a few tests?

Code quote:

extractRegistrarIdFromSessionInfo

[CydeWeys]

Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @ptkach and @weiminyu)

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 139 at r1 (raw file):

        .set(HttpHeaderNames.ACCEPT, EPP_CONTENT_TYPE);

    if (maybeRegistrarId.isPresent()

How is this checking if something is present that hasn't even been assigned yet? It's being reused from a previous request in this same connection, or what?

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 171 at r1 (raw file):

    try {
      String decodedString = new String(BaseEncoding.base64Url().decode(sessionInfo), US_ASCII);
      Pattern pattern = Pattern.compile("clientId=([^,\\s]+)?");

Make this compiled pattern a static final on the class, so it doesn't need to be compiled from scratch during each request.

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 178 at r1 (raw file):

        if (!maybeRegistrarIdMatch.equals("null")) {
          maybeRegistrarId = Optional.of(maybeRegistrarIdMatch);
          return maybeRegistrarId;

It's a little confusing that this helper method is both setting an instance variable, and returning that value. (And even worse, that the value is being extracted and set during a conditional check, which typically you wouldn't think would modify any state.) I'd suggest doing one or the other, not both.

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 182 at r1 (raw file):

      }

    } catch (Throwable e) {

This seems like a pretty broad catch, when all you really need to catch seems to be IllegalArgumentException thrown by BaseEncoding.decode()? Are there other exceptions at play here?

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 183 at r1 (raw file):

    } catch (Throwable e) {
      logger.atSevere().withCause(e).log("Failed to decode Base64 string %s", sessionInfo);

How about "Failed to decode session info from Base64"

And should we be logging the session info? Might that not put sensitive data in the logs that shouldn't be there?

[CydeWeys]

Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @ptkach and @weiminyu)

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 165 at r1 (raw file):

  private Optional<String> extractRegistrarIdFromSessionInfo(String sessionInfo) {
    if (sessionInfo == null) {

Annotate sessionInfo above with @Nullable if null is an expected value for it.

[ptkach]

Reviewable status: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @CydeWeys and @weiminyu)

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 139 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

How is this checking if something is present that hasn't even been assigned yet? It's being reused from a previous request in this same connection, or what?

The instance is alive as long as connection is

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 164 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

Add a few tests?

Done.

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 165 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Annotate sessionInfo above with @Nullable if null is an expected value for it.

What sessionInfo above? This is the value of the cookie, which can potentially be null

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 171 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Make this compiled pattern a static final on the class, so it doesn't need to be compiled from scratch during each request.

It's not compiled from scratch during each request. It only does it once when connection is established and then it's saved in maybeRegistrarId

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 178 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

It's a little confusing that this helper method is both setting an instance variable, and returning that value. (And even worse, that the value is being extracted and set during a conditional check, which typically you wouldn't think would modify any state.) I'd suggest doing one or the other, not both.

Fair enough

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 182 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

This seems like a pretty broad catch, when all you really need to catch seems to be IllegalArgumentException thrown by BaseEncoding.decode()? Are there other exceptions at play here?

The reason for broad catch was to eliminate any chance of runtime failures. This is not essential functionality which is on a hot pathway, so I tried to be little extra careful.

[CydeWeys]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @ptkach and @weiminyu)

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 165 at r1 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

What sessionInfo above? This is the value of the cookie, which can potentially be null

private Optional<String> extractRegistrarIdFromSessionInfo(String sessionInfo) {

should be

private Optional<String> extractRegistrarIdFromSessionInfo(@Nullable String sessionInfo) {

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 171 at r1 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

It's not compiled from scratch during each request. It only does it once when connection is established and then it's saved in maybeRegistrarId

What if the connection consists of thousands of invalid requests, none of which contain a registrar ID? Won't it keep re-compiling this pattern each time?

[ptkach]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @weiminyu)

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 165 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

private Optional<String> extractRegistrarIdFromSessionInfo(String sessionInfo) {

should be

private Optional<String> extractRegistrarIdFromSessionInfo(@Nullable String sessionInfo) {

Done

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 171 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

What if the connection consists of thousands of invalid requests, none of which contain a registrar ID? Won't it keep re-compiling this pattern each time?

This will only execute after epp login, so session should be valid and sessionInfo == null should pass, which will happen only once

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 183 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

How about "Failed to decode session info from Base64"

And should we be logging the session info? Might that not put sensitive data in the logs that shouldn't be there?

Done.

[CydeWeys]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @ptkach and @weiminyu)

---

proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java line 171 at r1 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

This will only execute after epp login, so session should be valid and sessionInfo == null should pass, which will happen only once

Right but can't a registrar send thousands of failing login commands all on the same session? Or is that not possible / does this run only after a successful login?

[CydeWeys]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @weiminyu)

[weiminyu]

Reviewed 2 of 3 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ptkach)