Uploaded image for project: 'Kafka'
  1. Kafka
  2. KAFKA-1682 Security for Kafka
  3. KAFKA-1477

add authentication layer and initial JKS x509 implementation for brokers, producers and consumer for network communication

Details

    • Sub-task
    • Status: Resolved
    • Major
    • Resolution: Duplicate
    • None
    • 0.9.0.0
    • security
    • None

    Attachments

      1. KAFKA-1477_2014-06-02_16:59:40.patch
        114 kB
        Ivan Lyutov
      2. KAFKA-1477_2014-06-02_17:24:26.patch
        114 kB
        Ivan Lyutov
      3. KAFKA-1477_2014-06-03_13:46:17.patch
        124 kB
        Ivan Lyutov
      4. KAFKA-1477_trunk.patch
        157 kB
        Ivan Lyutov
      5. KAFKA-1477.patch
        895 kB
        Ivan Lyutov
      6. KAFKA-1477-binary.patch
        157 kB
        Ivan Lyutov

      Issue Links

        Activity

          edgefox Ivan Lyutov added a comment -

          Created reviewboard https://reviews.apache.org/r/22131/
          against branch origin/trunk

          edgefox Ivan Lyutov added a comment - Created reviewboard https://reviews.apache.org/r/22131/ against branch origin/trunk
          edgefox Ivan Lyutov added a comment -

          Updated reviewboard https://reviews.apache.org/r/22131/
          against branch apache/trunk

          edgefox Ivan Lyutov added a comment - Updated reviewboard https://reviews.apache.org/r/22131/ against branch apache/trunk
          edgefox Ivan Lyutov added a comment -

          Updated reviewboard https://reviews.apache.org/r/22131/
          against branch apache/trunk

          edgefox Ivan Lyutov added a comment - Updated reviewboard https://reviews.apache.org/r/22131/ against branch apache/trunk
          edgefox Ivan Lyutov added a comment -

          Updated reviewboard https://reviews.apache.org/r/22131/
          against branch apache/trunk

          edgefox Ivan Lyutov added a comment - Updated reviewboard https://reviews.apache.org/r/22131/ against branch apache/trunk
          junrao Jun Rao added a comment -

          Thanks for the patch. A couple comments.

          1. Since this is a relatively big change on an important feature, could we add some design doc first so that we can have some discussion on the best way to approach the implementation?

          2. We have a new producer in trunk and the old producer will eventually be phased out. Perhaps the ssl implementation only needs to be added to the new producer?

          junrao Jun Rao added a comment - Thanks for the patch. A couple comments. 1. Since this is a relatively big change on an important feature, could we add some design doc first so that we can have some discussion on the best way to approach the implementation? 2. We have a new producer in trunk and the old producer will eventually be phased out. Perhaps the ssl implementation only needs to be added to the new producer?
          joestein Joe Stein added a comment -

          << 1. Since this is a relatively big change on an important feature, could we add some design doc first so that we can have some discussion on the best way to approach the implementation?

          Yup, will do. I will update the wiki.

          << 2. We have a new producer in trunk and the old producer will eventually be phased out. Perhaps the ssl implementation only needs to be added to the new producer?

          I am not opposed to this so we can speed the adoption of the new producer. Do we have a code path transition for folks going from old to new? Is ConsoleProducer's shiny new producer the best entry point for folks?

          joestein Joe Stein added a comment - << 1. Since this is a relatively big change on an important feature, could we add some design doc first so that we can have some discussion on the best way to approach the implementation? Yup, will do. I will update the wiki. << 2. We have a new producer in trunk and the old producer will eventually be phased out. Perhaps the ssl implementation only needs to be added to the new producer? I am not opposed to this so we can speed the adoption of the new producer. Do we have a code path transition for folks going from old to new? Is ConsoleProducer's shiny new producer the best entry point for folks?
          junrao Jun Rao added a comment -

          For most people, they will have to make code changes to use the new producer api. So, it's going to take a few releases before we can remove the old producer. However, we probably should just put the old producer mostly in maintenance mode w/o adding new features.

          junrao Jun Rao added a comment - For most people, they will have to make code changes to use the new producer api. So, it's going to take a few releases before we can remove the old producer. However, we probably should just put the old producer mostly in maintenance mode w/o adding new features.

          Ivan, Looks like the patch you attached to Jira yesterday is uploaded to review. Could you upload..? Can you also provide short summary of changes in last patch?

          erajasekar Rajasekar Elango added a comment - Ivan, Looks like the patch you attached to Jira yesterday is uploaded to review. Could you upload..? Can you also provide short summary of changes in last patch?
          edgefox Ivan Lyutov added a comment -

          Rajasekar, the previous patch failed to apply on latest trunk version(https://github.com/stealthly/kafka/tree/v0.8.2_KAFKA-1477). So, I had to do make the patch manually.
          There are still some items pending, such as making security off by default and more testing stuff. So, you can expect one more patch in the nearest future.

          edgefox Ivan Lyutov added a comment - Rajasekar, the previous patch failed to apply on latest trunk version( https://github.com/stealthly/kafka/tree/v0.8.2_KAFKA-1477 ). So, I had to do make the patch manually. There are still some items pending, such as making security off by default and more testing stuff. So, you can expect one more patch in the nearest future.

          lindong It doesn't look like review https://reviews.apache.org/r/22905/diff/ commented on this jira is not related to this functionality. Can you double check?

          erajasekar Rajasekar Elango added a comment - lindong It doesn't look like review https://reviews.apache.org/r/22905/diff/ commented on this jira is not related to this functionality. Can you double check?
          lindong Dong Lin added a comment -

          Rajasekar, I have submitted to this JIRA by mistake. Please ignore it or remove unrelated patch/link. Sorry about it.

          lindong Dong Lin added a comment - Rajasekar, I have submitted to this JIRA by mistake. Please ignore it or remove unrelated patch/link. Sorry about it.
          lindong Dong Lin added a comment -

          Hey Rajasekar, I have submitted to this JIRA by mistake. Please ignore it
          or remove unrelated patch/link. Sorry about it.

          On Tue, Jun 24, 2014 at 8:18 AM, Rajasekar Elango (JIRA) <jira@apache.org>

          –
          Lin, Dong
          Dept. of Computer & Information Science
          University of Pennsylvania

          lindong Dong Lin added a comment - Hey Rajasekar, I have submitted to this JIRA by mistake. Please ignore it or remove unrelated patch/link. Sorry about it. On Tue, Jun 24, 2014 at 8:18 AM, Rajasekar Elango (JIRA) <jira@apache.org> – Lin, Dong Dept. of Computer & Information Science University of Pennsylvania
          joestein Joe Stein added a comment -

          junrao While I think it make sense to not try to add too much to the existing producer and consumer they are being used in the wild quite heavily right now. There is real need for this feature (besides Salesforce others are doing security with Kafka outside of the upstream project and starting to-do things differently enough that the solution is becoming inconsistent) and a lot of this is really about the changes to the broker... Once the patch is committed then any client (Go, Python, etc) can implement SSL with the Broker. The existing producer/consumer can also be used out of the box for folks that want to flip the feature on now.

          Waiting for the new producer won't work IMHO from a product perspective because then we only delivered half the solution (the other half being consuming over SSL). We can get the feature in now for something that folks are using already (this is an upgrade to Raja's original work to latest trunk). I understand this may extend the life of the existing producer/consumer a little bit until the new producer and consumer support the feature too. I think the reality though is that existing producer / consumer will be around as folks will be slow to upgrade though I do expect them to eventually move to "shiny new" once it becomes less new or they have a real need for the change (some do not).

          If there are no objections (-1 s) to the patch Ivan posted he should have this done finalized within the next 2-3 weeks (minor things). Once done with review and testing I expect to +1 and commit the patch. I also appreciate the extent of the change and also appreciate any effort that can be made to help vet this change to make sure Kafka keeps doing everything that everyone expects Kafka to be doing when the feature is not on.

          joestein Joe Stein added a comment - junrao While I think it make sense to not try to add too much to the existing producer and consumer they are being used in the wild quite heavily right now. There is real need for this feature (besides Salesforce others are doing security with Kafka outside of the upstream project and starting to-do things differently enough that the solution is becoming inconsistent) and a lot of this is really about the changes to the broker... Once the patch is committed then any client (Go, Python, etc) can implement SSL with the Broker. The existing producer/consumer can also be used out of the box for folks that want to flip the feature on now. Waiting for the new producer won't work IMHO from a product perspective because then we only delivered half the solution (the other half being consuming over SSL). We can get the feature in now for something that folks are using already (this is an upgrade to Raja's original work to latest trunk). I understand this may extend the life of the existing producer/consumer a little bit until the new producer and consumer support the feature too. I think the reality though is that existing producer / consumer will be around as folks will be slow to upgrade though I do expect them to eventually move to "shiny new" once it becomes less new or they have a real need for the change (some do not). If there are no objections (-1 s) to the patch Ivan posted he should have this done finalized within the next 2-3 weeks (minor things). Once done with review and testing I expect to +1 and commit the patch. I also appreciate the extent of the change and also appreciate any effort that can be made to help vet this change to make sure Kafka keeps doing everything that everyone expects Kafka to be doing when the feature is not on.
          junrao Jun Rao added a comment -

          Joe,

          I'd rather that we don't rush to get this patch in. This is because (1) We have accumulated a lot of changes for the next release (0.8.2) in trunk, including Kafka-based offset management and the new clients. It's going to be difficult to absorb big patches like this in the same release. (2) I felt that we haven't had enough discussion on the implementation. I took at look at the changes that you made in https://cwiki.apache.org/confluence/display/KAFKA/Security. What's in there are mostly feature requirements. I was expecting to see a design doc of the implementation. I am no security expert, but I have questions like (a) should we use two separate server ports so that we can support both secure and non-secure clients in the same cluster (b) is a local secure file the right way to store security credentials? If we have a more concrete design doc, perhaps more people with security experience can chime in and help us make the right design choice.

          We can also discuss whether the security feature should only be done on the new clients or not. At this moment, we are trying to put the old clients mostly in maintenance mode and will only try to fix blocker issues. The more we need to patch on the old clients, the more the maintenance work. Also, the new consumer will remove the ZK dependence. That potentially will make adding the security feature a bit easier on the consumer.

          So, I recommend that we start working on a more concrete design doc first and then solicit some feedback. We can probably target this feature in 0.9.

          junrao Jun Rao added a comment - Joe, I'd rather that we don't rush to get this patch in. This is because (1) We have accumulated a lot of changes for the next release (0.8.2) in trunk, including Kafka-based offset management and the new clients. It's going to be difficult to absorb big patches like this in the same release. (2) I felt that we haven't had enough discussion on the implementation. I took at look at the changes that you made in https://cwiki.apache.org/confluence/display/KAFKA/Security . What's in there are mostly feature requirements. I was expecting to see a design doc of the implementation. I am no security expert, but I have questions like (a) should we use two separate server ports so that we can support both secure and non-secure clients in the same cluster (b) is a local secure file the right way to store security credentials? If we have a more concrete design doc, perhaps more people with security experience can chime in and help us make the right design choice. We can also discuss whether the security feature should only be done on the new clients or not. At this moment, we are trying to put the old clients mostly in maintenance mode and will only try to fix blocker issues. The more we need to patch on the old clients, the more the maintenance work. Also, the new consumer will remove the ZK dependence. That potentially will make adding the security feature a bit easier on the consumer. So, I recommend that we start working on a more concrete design doc first and then solicit some feedback. We can probably target this feature in 0.9.
          joestein Joe Stein added a comment - - edited

          Hi Jun, maybe what we (myself, Ivan and the other developers working @ BDOSS) should do then is keep this patch up to date (from a rebase / fix perspective) so folks that are using it already (as there are some folks doing that) and folks that can't use Kafka with out it (there are folks in that camp too) and continue to keep it updated so they still get the benefits coming in 0.8.2 (and moving onwards until/if it gets upstream). It requires some more work on our part and on theirs but that is the trade off we would have to accept. Then we can add to the design doc as you suggest and take changes that come up from there and work them back into the patch (or create a new one) as appropriate and release it as the team can agree for the community needs.

          Another option to the dangling patch approach (which I have seen be an issue in projects) is a security branch. This approach I have seen be problematic from a community perspective especially with voting and releasing. I am not sure if it was the project team members that caused this or the approach they took or something else, unsure. I would be cautious with going the branch route and I don't know dunno if it would be better but maybe? I also don't know if there were enough other pmc members that would vote for a branch release (regardless of what it was) and then also if they wold vote these changes in a branch release or what folks think of this in general. Having something available from an Apache release perspective has certain usefulness within organizations that you can't get any other way.

          From my perspective I want to-do what is going to be best for the community and the project. Personally I am happy to spend my time and commit BDOSS resources to apply the patch when we need to for our use or our clients need for it... I can't speak for others though,

          Per the port - there may be use case(s) that you need to have both the secure and non secure port on at the same time. Maybe what we do is make it configurable so you can turn off the none secure port along with enabling a secure port or even enable both. I know having only the secure and authenticated port on is a use case.

          joestein Joe Stein added a comment - - edited Hi Jun, maybe what we (myself, Ivan and the other developers working @ BDOSS) should do then is keep this patch up to date (from a rebase / fix perspective) so folks that are using it already (as there are some folks doing that) and folks that can't use Kafka with out it (there are folks in that camp too) and continue to keep it updated so they still get the benefits coming in 0.8.2 (and moving onwards until/if it gets upstream). It requires some more work on our part and on theirs but that is the trade off we would have to accept. Then we can add to the design doc as you suggest and take changes that come up from there and work them back into the patch (or create a new one) as appropriate and release it as the team can agree for the community needs. Another option to the dangling patch approach (which I have seen be an issue in projects) is a security branch. This approach I have seen be problematic from a community perspective especially with voting and releasing. I am not sure if it was the project team members that caused this or the approach they took or something else, unsure. I would be cautious with going the branch route and I don't know dunno if it would be better but maybe? I also don't know if there were enough other pmc members that would vote for a branch release (regardless of what it was) and then also if they wold vote these changes in a branch release or what folks think of this in general. Having something available from an Apache release perspective has certain usefulness within organizations that you can't get any other way. From my perspective I want to-do what is going to be best for the community and the project. Personally I am happy to spend my time and commit BDOSS resources to apply the patch when we need to for our use or our clients need for it... I can't speak for others though, Per the port - there may be use case(s) that you need to have both the secure and non secure port on at the same time. Maybe what we do is make it configurable so you can turn off the none secure port along with enabling a secure port or even enable both. I know having only the secure and authenticated port on is a use case.
          jkreps Jay Kreps added a comment -

          Yeah Hey Joe, that is a great offer. I don't personally have a big preference between patch and branch since either way I suppose you end up managing it via git.

          I don't think there is any concern about upgrading the old producer and consumer. If folks are willing to do that, and we have sufficient test coverage for the security related stuff, then that is great.

          To second what Jun is saying I think for big user-facing stuff like this it is good if we can avoid incremental development. Even though that is good for us developers, I think it can be frustrating for infrastructure users if things keep churning under them.

          So I think what we need to do is what Jun described. We need to get a document together that fully describes the cases we need to support, and how people will use these features, as well as the relevant details of implementation and how we will handle ongoing testing in this area. This will let us have something to circulate to get broad consensus among users, who have very different environments, so that what we build will work for all of them (or at least the subset that makes sense). When we have this figured out I think it may well prove possible to take pieces of functionality a bit at a time as we will know where we are going and not have to worry about doing and then redoing things as we evolve our approach.

          I think the stakeholders here are at least the people who have expressed interest so far which to my knowledge is BDOSS, Salesforce, LinkedIn, Cloudera, and Hortonworks. But I expect there are a substantial number of others who would have input.

          I'd like to work with you to flesh out these requirements more. I am pretty overbooked this week, but should be more free to start next week if that would work.

          jkreps Jay Kreps added a comment - Yeah Hey Joe, that is a great offer. I don't personally have a big preference between patch and branch since either way I suppose you end up managing it via git. I don't think there is any concern about upgrading the old producer and consumer. If folks are willing to do that, and we have sufficient test coverage for the security related stuff, then that is great. To second what Jun is saying I think for big user-facing stuff like this it is good if we can avoid incremental development. Even though that is good for us developers, I think it can be frustrating for infrastructure users if things keep churning under them. So I think what we need to do is what Jun described. We need to get a document together that fully describes the cases we need to support, and how people will use these features, as well as the relevant details of implementation and how we will handle ongoing testing in this area. This will let us have something to circulate to get broad consensus among users, who have very different environments, so that what we build will work for all of them (or at least the subset that makes sense). When we have this figured out I think it may well prove possible to take pieces of functionality a bit at a time as we will know where we are going and not have to worry about doing and then redoing things as we evolve our approach. I think the stakeholders here are at least the people who have expressed interest so far which to my knowledge is BDOSS, Salesforce, LinkedIn, Cloudera, and Hortonworks. But I expect there are a substantial number of others who would have input. I'd like to work with you to flesh out these requirements more. I am pretty overbooked this week, but should be more free to start next week if that would work.

          Very Good Idea joestein . We (@ salesforce) like secure features to be rebased with latest release so that we can get benefits of using latest version. if required we can help with merging/testing etc. In parallel, we should also plan to merge this to trunk (after design spec review) to avoid cost of merging & testing for each rebase.

          For the port, our use case is to run it in secure mode or in non-secure mode. We thought about supporting both secure and non-secure at different ports, but it looked more complicated to implement, so we went with simple flag to turn on secure mode.

          Thanks,
          Raja.

          erajasekar Rajasekar Elango added a comment - Very Good Idea joestein . We (@ salesforce) like secure features to be rebased with latest release so that we can get benefits of using latest version. if required we can help with merging/testing etc. In parallel, we should also plan to merge this to trunk (after design spec review) to avoid cost of merging & testing for each rebase. For the port, our use case is to run it in secure mode or in non-secure mode. We thought about supporting both secure and non-secure at different ports, but it looked more complicated to implement, so we went with simple flag to turn on secure mode. Thanks, Raja.
          joestein Joe Stein added a comment -

          jkreps agreed, next week sounds good

          joestein Joe Stein added a comment - jkreps agreed, next week sounds good
          gwenshap Gwen Shapira added a comment -

          joestein jkreps So the next steps are fleshing out the exact requirements and what we will support? And my understanding is that this will be done in public, probably in the wiki?

          I'm interested in adding the feedback that I've been getting from our customers into the mix.

          gwenshap Gwen Shapira added a comment - joestein jkreps So the next steps are fleshing out the exact requirements and what we will support? And my understanding is that this will be done in public, probably in the wiki? I'm interested in adding the feedback that I've been getting from our customers into the mix.
          joestein Joe Stein added a comment -

          Hey gwenshap take a look where we are so far https://cwiki.apache.org/confluence/display/KAFKA/Security very much welcome feedback. (just granted you access to the wiki).

          joestein Joe Stein added a comment - Hey gwenshap take a look where we are so far https://cwiki.apache.org/confluence/display/KAFKA/Security very much welcome feedback. (just granted you access to the wiki).

          CloudFlare is also very interested in this work. We are most interested in encryption and mutual authentication over TLS. I think it would be useful to be have a single broker support both an unencrypted and an encrypted port (producer on local network sends unencrypted, consumers on remote network consume encrypted).

          We're mainly using this to move log events between data centers.

          We've managed to kludge this together on top of an existing Kafka setup by using stunnel/haproxy 1.5 to do the encryption part along the lines described here:

          https://mail-archives.apache.org/mod_mbox/kafka-users/201405.mbox/%3CCABgmubZT3=n780LxmVoUNZz6sHgVPD_T8BN-WfOGE5kXYSVrxw@mail.gmail.com%3E

          but this will only work with a single broker and carefully matching the hostname/port in stunnel with the MetadataResponse, since setting advertised.host.name= and advertised.port= to the haproxy host and port causes the controller to try to use it that host/port for metadata, which doesn't work (as one would expect).

          To be able to support multiple brokers (we only need maybe 1, 2 or 3 in this setup), we will probably tweak our consumer code to remap the advertised host/port and connect over TLS directly (without stunnel).

          fullung Albert Strasheim added a comment - CloudFlare is also very interested in this work. We are most interested in encryption and mutual authentication over TLS. I think it would be useful to be have a single broker support both an unencrypted and an encrypted port (producer on local network sends unencrypted, consumers on remote network consume encrypted). We're mainly using this to move log events between data centers. We've managed to kludge this together on top of an existing Kafka setup by using stunnel/haproxy 1.5 to do the encryption part along the lines described here: https://mail-archives.apache.org/mod_mbox/kafka-users/201405.mbox/%3CCABgmubZT3=n780LxmVoUNZz6sHgVPD_T8BN-WfOGE5kXYSVrxw@mail.gmail.com%3E but this will only work with a single broker and carefully matching the hostname/port in stunnel with the MetadataResponse, since setting advertised.host.name= and advertised.port= to the haproxy host and port causes the controller to try to use it that host/port for metadata, which doesn't work (as one would expect). To be able to support multiple brokers (we only need maybe 1, 2 or 3 in this setup), we will probably tweak our consumer code to remap the advertised host/port and connect over TLS directly (without stunnel).
          edgefox Ivan Lyutov added a comment -

          Updated patch to current trunk

          edgefox Ivan Lyutov added a comment - Updated patch to current trunk

          Thanks edgefox , could you upload patch to review so that it is easier to review and add comments ?

          erajasekar Rajasekar Elango added a comment - Thanks edgefox , could you upload patch to review so that it is easier to review and add comments ?

          lagivan Thanks for picking this up. The review looks good, I just left one comment to set default value of secure flag to false. Also do you have this code available on a git repo which I can try to build and test..?

          erajasekar Rajasekar Elango added a comment - lagivan Thanks for picking this up. The review looks good, I just left one comment to set default value of secure flag to false. Also do you have this code available on a git repo which I can try to build and test..?
          edgefox Ivan Lyutov added a comment -

          I have updated the reviewboard https://reviews.apache.org/r/22131/

          edgefox Ivan Lyutov added a comment - I have updated the reviewboard https://reviews.apache.org/r/22131/
          edgefox Ivan Lyutov added a comment -

          Rajasekar Elango, ssl branch is located here http://github.com/edgefox/kafka/tree/kafka-ssl

          edgefox Ivan Lyutov added a comment - Rajasekar Elango, ssl branch is located here http://github.com/edgefox/kafka/tree/kafka-ssl
          gwenshap Gwen Shapira added a comment -

          Quick question regarding the patch:
          In the wiki we mention using a separate port for SSL. From what I've seen this patch doesn't add an SSL port, it simply allows making the existing port "secure". Is that correct?

          gwenshap Gwen Shapira added a comment - Quick question regarding the patch: In the wiki we mention using a separate port for SSL. From what I've seen this patch doesn't add an SSL port, it simply allows making the existing port "secure". Is that correct?
          edgefox Ivan Lyutov added a comment - - edited

          Gwen Shapira, this patch is for securing existing port and nothing more. Of course, we plan to add support for simultaneous usage of secure and non-secure ports in future.

          edgefox Ivan Lyutov added a comment - - edited Gwen Shapira, this patch is for securing existing port and nothing more. Of course, we plan to add support for simultaneous usage of secure and non-secure ports in future.

          This is great edgefox . I did a quick test and able to get it working in both secure and non-secure mode. But I noticed that we original used format <broker>:<port>:<secureflag> to specify brokerlist for producer, now looks like new secure property is added to producer config and specified separately. Do you have usage documented anywhere? I also noticed a change to security.config.file option in console producer and left a comment in review.

          erajasekar Rajasekar Elango added a comment - This is great edgefox . I did a quick test and able to get it working in both secure and non-secure mode. But I noticed that we original used format <broker>:<port>:<secureflag> to specify brokerlist for producer, now looks like new secure property is added to producer config and specified separately. Do you have usage documented anywhere? I also noticed a change to security.config.file option in console producer and left a comment in review.
          gwenshap Gwen Shapira added a comment -

          Another question:
          The change mentions that there will be config/server.security.properties file.
          I did not see it in the patch, so I'm wondering what goes in there.

          One of the options I'd like to see is whether client certificates are required or not (i.e. choosing whether we want to authenticate or just encrypt).

          gwenshap Gwen Shapira added a comment - Another question: The change mentions that there will be config/server.security.properties file. I did not see it in the patch, so I'm wondering what goes in there. One of the options I'd like to see is whether client certificates are required or not (i.e. choosing whether we want to authenticate or just encrypt).

          gwenshap config/server.security.properties is already in review. You should able to set need.client.auth=false to disable mutual authentication (client will only authenticate server, server won't authenticate server).

          edgefox FYI. I left one more comment to ZkUtils.java.

          erajasekar Rajasekar Elango added a comment - gwenshap config/server.security.properties is already in review. You should able to set need.client.auth=false to disable mutual authentication (client will only authenticate server, server won't authenticate server). edgefox FYI. I left one more comment to ZkUtils.java.
          joestein Joe Stein added a comment -

          We need to have it support configurable ports and we can then do on/off for secured and non/secured or even have both on at same time. advertised.port.ssl should be added along with port.ssl

          joestein Joe Stein added a comment - We need to have it support configurable ports and we can then do on/off for secured and non/secured or even have both on at same time. advertised.port.ssl should be added along with port.ssl
          junrao Jun Rao added a comment -

          Joe,

          Just to clarify. Have we agreed to support security only on the new (java) client? If so, this patch needs to be reworked based on the new Selector in the client, right?

          junrao Jun Rao added a comment - Joe, Just to clarify. Have we agreed to support security only on the new (java) client? If so, this patch needs to be reworked based on the new Selector in the client, right?
          joestein Joe Stein added a comment -

          Jun,

          Getting this to work in the new (java) client is going to take some more work integrating a secure nio version. Ivan is going to start later next week on that after this last configuration item is done. I created separate tickets KAFKA-1690 and KAFKA-1691 for the new (java) client work.

          I am +1 to committing this patch to trunk after the server configuration item is done and working how folks expect. This gives the broker support so we can start to get the existing clients (including third party clients e.g. https://github.com/Shopify/sarama/pull/156 ) to hook in now for security features as we are moving ahead with them overall. I see the existing (scala and third party) clients as stable clients we are incrementally changing to confirm compatibility with each other, the broker and new (java) clients too. We can drop support once they are not being used and folks have upgraded (or on an upgrade path at least) but I don't think we are there yet so putting this in now make sense.

          joestein Joe Stein added a comment - Jun, Getting this to work in the new (java) client is going to take some more work integrating a secure nio version. Ivan is going to start later next week on that after this last configuration item is done. I created separate tickets KAFKA-1690 and KAFKA-1691 for the new (java) client work. I am +1 to committing this patch to trunk after the server configuration item is done and working how folks expect. This gives the broker support so we can start to get the existing clients (including third party clients e.g. https://github.com/Shopify/sarama/pull/156 ) to hook in now for security features as we are moving ahead with them overall. I see the existing (scala and third party) clients as stable clients we are incrementally changing to confirm compatibility with each other, the broker and new (java) clients too. We can drop support once they are not being used and folks have upgraded (or on an upgrade path at least) but I don't think we are there yet so putting this in now make sense.
          jkreps Jay Kreps added a comment -

          I took a look at this patch in a little more detail, I think there is likely a fair bit of work to do before we can check this in.

          For example, some things that concern me: The SSLSocketChannel class extends SocketChannel. We seem to be simulating blocking on a non-blocking socket using sleep calls in a loop. Then even lots of minor things like channelFor is doing handshaking and some odd unfinished looking code.

          I suspect some of this may be done this way to minimize impact to existing code since it was being maintained as a patch, but that won't make sense once it is committed.

          What about this as a path forward. Let's take this patch and extract just the server-side SSL support in SocketServer and try to get that into shape to be something we can commit. I think we can do this without simultaneously doing the clients. I think if we try to do this all at once we aren't going to get there. We can test this by adding to SocketServerTest and just using a blocking SSL connection. Here is what I think we need to do:
          1. Do we need SSLSocketChannel? I think as long as the acceptor completes the handshake from then on all that is needed is to wrap/unwrap bytes, right?
          2. Modify the acceptor in SocketServer to do non-blocking handling of the SSL handshake. By the time the socket is accepted and handed over to the processor the ssl handshake should be complete.
          3. Create some kind of generic interface for wrap/upwrap (SecurityCodec?) as we will need to implement this for both ssl and for kerberos. This interface will wrap the SSLEngine (or SASL engine) associated with a given connection.

          jkreps Jay Kreps added a comment - I took a look at this patch in a little more detail, I think there is likely a fair bit of work to do before we can check this in. For example, some things that concern me: The SSLSocketChannel class extends SocketChannel. We seem to be simulating blocking on a non-blocking socket using sleep calls in a loop. Then even lots of minor things like channelFor is doing handshaking and some odd unfinished looking code. I suspect some of this may be done this way to minimize impact to existing code since it was being maintained as a patch, but that won't make sense once it is committed. What about this as a path forward. Let's take this patch and extract just the server-side SSL support in SocketServer and try to get that into shape to be something we can commit. I think we can do this without simultaneously doing the clients. I think if we try to do this all at once we aren't going to get there. We can test this by adding to SocketServerTest and just using a blocking SSL connection. Here is what I think we need to do: 1. Do we need SSLSocketChannel? I think as long as the acceptor completes the handshake from then on all that is needed is to wrap/unwrap bytes, right? 2. Modify the acceptor in SocketServer to do non-blocking handling of the SSL handshake. By the time the socket is accepted and handed over to the processor the ssl handshake should be complete. 3. Create some kind of generic interface for wrap/upwrap (SecurityCodec?) as we will need to implement this for both ssl and for kerberos. This interface will wrap the SSLEngine (or SASL engine) associated with a given connection.

          People

            edgefox Ivan Lyutov
            joestein Joe Stein
            Votes:
            8 Vote for this issue
            Watchers:
            24 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: