flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From vijikarthi <...@git.apache.org>
Subject [GitHub] flink issue #2425: FLINK-3930 Added shared secret based authorization for Fl...
Date Wed, 19 Oct 2016 23:31:40 GMT
Github user vijikarthi commented on the issue:

    Thanks @mxm for the review. I will incorporate your feedback and attach the patch.
    When security is enabled, encryption should also be turned on by default. Otherwise we
will transmit plain-text passwords over the wire.
    Yes it makes sense. I will make a conditional check and throw an error if encryption is
not enabled when security is enabled?
    Should there be too modes for network transmission, 1) with cookie, one without? Do we
need 32 bits for the cookie length? We should be precise about the maximum length. I saw it
is set to 1024 in other places.
    Yes, max cookie length validation is 1024. I will change the code where the buffer length
was allocated to a high value, instead it will use the byte array length read from the message.

    Should we really always send the cookie for every message? The security document mentions
in T2-3 that we only want to authorize upon the first connection.
    Yes, we took the approach to pass secure cookie for every message to keep minimal changes
to the current design
    Why do we transmit the cookie to the client? This seems like a major security concern.
The client should always provide the cookie.
        edit: I see this has been specified in the document in T2-4. Still, I wonder if it
would make sense to simply add this now because the workaround to fetch the cookie from the
JobManager doesn't look appealing.
    Good catch. I forgot to revert the code after the merge and it is not required. Will fix
    You added a Yarn specific cookie option which should be part of the general options instead.
    It is added since secure cookie can be supplied when using both Yarn session CLI as well
as Flink CLI. I have provided detailed explanation in one of the comments.
    You've introduce a new config file to persist the app state. We already have the Yarn
properties file for that.
    I have provided explanation on why we need new config file in one of the comments. Please
take a look.

If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.

View raw message