Uploaded image for project: 'Mesos'
  1. Mesos
  2. MESOS-3560

JSON-based credential files do not work correctly

    Details

    • Target Version/s:
    • Sprint:
      Mesosphere Sprint 20, Mesosphere Sprint 21
    • Story Points:
      1

      Description

      Specifying the following credentials file:

      {
        “credentials”: [
          {
            “principal”: “user”,
            “secret”: “password”
          }
        ]
      }
      

      Then hitting a master endpoint with:

      curl -i -u “user:password” ...
      

      Does not work. This is contrary to the text-based credentials file which works:

      user password
      

      Currently, the password in a JSON-based credentials file needs to be base64-encoded in order for it to work:

      {
        “credentials”: [
          {
            “principal”: “user”,
            “secret”: “cGFzc3dvcmQ=”
          }
        ]
      }
      

        Issue Links

          Activity

          Hide
          srowen Sean Owen added a comment -

          This ends up causing a problem in Spark, for later when Mesos gets updated in Spark: https://issues.apache.org/jira/browse/SPARK-12501. Ideally the old field is also supported, but hey.

          Show
          srowen Sean Owen added a comment - This ends up causing a problem in Spark, for later when Mesos gets updated in Spark: https://issues.apache.org/jira/browse/SPARK-12501 . Ideally the old field is also supported, but hey.
          Hide
          srowen Sean Owen added a comment -

          This ends up causing a problem in Spark, for later when Mesos gets updated in Spark: https://issues.apache.org/jira/browse/SPARK-12501. Ideally the old field is also supported, but hey.

          Show
          srowen Sean Owen added a comment - This ends up causing a problem in Spark, for later when Mesos gets updated in Spark: https://issues.apache.org/jira/browse/SPARK-12501 . Ideally the old field is also supported, but hey.
          Hide
          ijimenez Isabel Jimenez added a comment -

          Neil Conway I sent an email to the dev mailing list to make a census on who this might perturb. There wasn't any response from users being concerned about this. I'm posting on RB the changes to upgrades.md and will up the email one more time.

          Show
          ijimenez Isabel Jimenez added a comment - Neil Conway I sent an email to the dev mailing list to make a census on who this might perturb. There wasn't any response from users being concerned about this. I'm posting on RB the changes to upgrades.md and will up the email one more time.
          Hide
          neilc Neil Conway added a comment -

          Are there any backward compatibility concerns here?

          At minimum, I think this change should be described in docs/upgrades.md, since the generated code will typically be different (e.g., this change requires modifications to the go-mesos bindings).

          Show
          neilc Neil Conway added a comment - Are there any backward compatibility concerns here? At minimum, I think this change should be described in docs/upgrades.md, since the generated code will typically be different (e.g., this change requires modifications to the go-mesos bindings).
          Hide
          mcypark Michael Park added a comment -
          commit a21d41f136e5000ea6ac2fbeace738579ce6df55
          Author: Isabel Jimenez <contact@isabeljimenez.com>
          Date:   Tue Oct 13 20:26:26 2015 +0200
          
              Changed secret field in V1 `Credential` from `bytes` to `string`
          
              Review: https://reviews.apache.org/r/39099
          
          Show
          mcypark Michael Park added a comment - commit a21d41f136e5000ea6ac2fbeace738579ce6df55 Author: Isabel Jimenez <contact@isabeljimenez.com> Date: Tue Oct 13 20:26:26 2015 +0200 Changed secret field in V1 `Credential` from `bytes` to `string` Review: https://reviews.apache.org/r/39099
          Hide
          mcypark Michael Park added a comment -
          commit 38dbadc944f56e24725fba102bbd2db76cb31228
          Author: Isabel Jimenez <contact@isabeljimenez.com>
          Date:   Tue Oct 13 10:47:24 2015 +0200
          
              Changed secret field in `Credential` from `bytes` to `string`
          
              When decoding the JSON credential file into the `Credential` protobuf
              object, the `secret` which is in `bytes` is mapped into base64 string
              automatically by protobuf from JSON. This leads to unintended behavior,
              forcing users to encode in base64 their secret when wanting to pass
              a JSON file to the --credentials flag.
          
              Review: https://reviews.apache.org/r/39098
          
          Show
          mcypark Michael Park added a comment - commit 38dbadc944f56e24725fba102bbd2db76cb31228 Author: Isabel Jimenez <contact@isabeljimenez.com> Date: Tue Oct 13 10:47:24 2015 +0200 Changed secret field in `Credential` from `bytes` to `string` When decoding the JSON credential file into the `Credential` protobuf object, the `secret` which is in `bytes` is mapped into base64 string automatically by protobuf from JSON. This leads to unintended behavior, forcing users to encode in base64 their secret when wanting to pass a JSON file to the --credentials flag. Review: https://reviews.apache.org/r/39098
          Show
          ijimenez Isabel Jimenez added a comment - https://reviews.apache.org/r/39098/
          Hide
          marco-mesos Marco Massenzio added a comment -

          Sure - not a problem.
          Do we need to take extra steps to ensure we don't break folks during upgrades?

          Show
          marco-mesos Marco Massenzio added a comment - Sure - not a problem. Do we need to take extra steps to ensure we don't break folks during upgrades?
          Hide
          mcypark Michael Park added a comment -

          From what I can tell, what Adam B mentioned in his presentation was explaining why the secret field is optional, not why the type of secret is bytes. That is, Kerberos for example passes its tickets out-of-band, so it doesn't use the secret part of Credential, just principal.

          I would much prefer to actually solve this problem now rather than just documenting the behavior. Since the current behavior is inconsistent and unintended, we'll end up breaking more people later when we go to fix it.

          Show
          mcypark Michael Park added a comment - From what I can tell, what Adam B mentioned in his presentation was explaining why the secret field is optional , not why the type of secret is bytes . That is, Kerberos for example passes its tickets out-of-band, so it doesn't use the secret part of Credential , just principal . I would much prefer to actually solve this problem now rather than just documenting the behavior. Since the current behavior is inconsistent and unintended, we'll end up breaking more people later when we go to fix it.
          Hide
          marco-mesos Marco Massenzio added a comment - - edited

          BTW - the approach of just documenting the fact that the string needs to be base64-encoded seems to me the easiest, let's just document it (maybe adding a suggestion to use openss base64 utility to encode/decode).

          Show
          marco-mesos Marco Massenzio added a comment - - edited BTW - the approach of just documenting the fact that the string needs to be base64-encoded seems to me the easiest, let's just document it (maybe adding a suggestion to use openss base64 utility to encode/decode).
          Hide
          marco-mesos Marco Massenzio added a comment -

          cc: Adam B

          I think the reasoning behind using a bytes field instead of a string was to enable other modules to use auth secrets, other than just passwords (eg, Tokens, keys, etc.)

          At least according to Adam's MesosCon presentation.

          Show
          marco-mesos Marco Massenzio added a comment - cc: Adam B I think the reasoning behind using a bytes field instead of a string was to enable other modules to use auth secrets, other than just passwords (eg, Tokens, keys, etc.) At least according to Adam's MesosCon presentation .
          Hide
          mcypark Michael Park added a comment - - edited

          Benjamin Mahler Looking back in git history, it looks like it was introduced in this commit while SASL was being added. From what I can see, it seems like changing secret to string would be fine. But I need to do some digging to make sure the fact that bytes is a ByteString whereas string is String in Java doesn't affect any communication with SASL and such.

          I'm happy to take the approach of doing the research on compatibility then changing bytes to string.

          Show
          mcypark Michael Park added a comment - - edited Benjamin Mahler Looking back in git history, it looks like it was introduced in this commit while SASL was being added. From what I can see, it seems like changing secret to string would be fine. But I need to do some digging to make sure the fact that bytes is a ByteString whereas string is String in Java doesn't affect any communication with SASL and such. I'm happy to take the approach of doing the research on compatibility then changing bytes to string .
          Hide
          bmahler Benjamin Mahler added a comment -

          Why is 'secret' a 'bytes' field? Can it be binary data?

          If it's enough for the secret to be a UTF-8 string, we should just change from 'bytes' to 'string'.

          Show
          bmahler Benjamin Mahler added a comment - Why is 'secret' a 'bytes' field? Can it be binary data? If it's enough for the secret to be a UTF-8 string, we should just change from 'bytes' to 'string'.
          Hide
          mcypark Michael Park added a comment -

          Isabel Jimenez Right, our JSON => protobuf parser decodes the input for a bytes field. I was thinking the simplest fix would be to base64 encode the password strings of the JSON input before passing it over to the protobuf parser, and things will work just fine.

          I'm not exactly sure what plans we have going forward, but this seems the simplest approach to me. What do you think?

          Show
          mcypark Michael Park added a comment - Isabel Jimenez Right, our JSON => protobuf parser decodes the input for a bytes field. I was thinking the simplest fix would be to base64 encode the password strings of the JSON input before passing it over to the protobuf parser, and things will work just fine. I'm not exactly sure what plans we have going forward, but this seems the simplest approach to me. What do you think?
          Hide
          ijimenez Isabel Jimenez added a comment -

          Michael Park When decoding the JSON credential file into the Credential protobuf object, the 'secret' which is in 'bytes' is mapped into base64 string automatically by protobuf from JSON (e.g proto3 and same in proto2 https://developers.google.com/protocol-buffers/docs/proto3#json)
          Which would be preferable, changing the protobuf definition so 'secret' can be a string? I don't really see why this is 'bytes'.
          Or try to find a hack for this to work? my assumption is that since 'secret' is bytes, when decoding JSON, protobuf is expecting base64 and anything else will not be properly processed for us to just simply do the necessary base64 decodes/encodes in the code. Thoughts?

          Show
          ijimenez Isabel Jimenez added a comment - Michael Park When decoding the JSON credential file into the Credential protobuf object, the 'secret' which is in 'bytes' is mapped into base64 string automatically by protobuf from JSON (e.g proto3 and same in proto2 https://developers.google.com/protocol-buffers/docs/proto3#json ) Which would be preferable, changing the protobuf definition so 'secret' can be a string? I don't really see why this is 'bytes'. Or try to find a hack for this to work? my assumption is that since 'secret' is bytes, when decoding JSON, protobuf is expecting base64 and anything else will not be properly processed for us to just simply do the necessary base64 decodes/encodes in the code. Thoughts?

            People

            • Assignee:
              ijimenez Isabel Jimenez
              Reporter:
              mcypark Michael Park
              Shepherd:
              Michael Park
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile