On Mon, 25 Mar 2013, Alex Elder wrote: > On 03/19/2013 06:08 PM, Sage Weil wrote: > > The 'cephx' auth protocol provides mutual authenticate for client and > > server. However, as the client, we were not verifying that the server > > auth reply was in fact authentic. Although the infrastructure to do so was > > all in place, we neglected to actually call the function to do it. Fix! > > > > This resolves http://tracker.ceph.com/issues/2429. > > I can't comment on the correctness of putting this check here > (but it looks reasonable to me). But the patch looks good. > Minor comments for you to consider below, but otherwise: > > Reviewed-by: Alex Elder <elder@xxxxxxxxxxx> > > > Reported-by: Alex Elder <elder@xxxxxxxxxxx> > > Signed-off-by: Sage Weil <sage@xxxxxxxxxxx> > > --- > > net/ceph/messenger.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c > > index 19af956..664adb1 100644 > > --- a/net/ceph/messenger.c > > +++ b/net/ceph/messenger.c > > @@ -1954,10 +1954,27 @@ static int process_connect(struct ceph_connection *con) > > u64 sup_feat = con->msgr->supported_features; > > u64 req_feat = con->msgr->required_features; > > u64 server_feat = le64_to_cpu(con->in_reply.features); > > + int authorizer_len = le32_to_cpu(con->in_reply.authorizer_len); > > int ret; > > > > dout("process_connect on %p tag %d\n", con, (int)con->in_tag); > > > > + if (authorizer_len && con->ops->verify_authorizer_reply) { > > Don't bother checking the method pointer, use the helper > below instead. > > > + mutex_unlock(&con->mutex); > > + ret = con->ops->verify_authorizer_reply(con, authorizer_len); > > Use the helper function. This is actually the messenger con ops, not the auth ops; inside this method (in mon_client.c and osd_client.c) we eventually call the ceph_auth_* method. > > > + mutex_lock(&con->mutex); > > + if (con->state != CON_STATE_NEGOTIATING) > > + return -EAGAIN; > > An assertion that we were in that state before dropping the > mutex would communicate to the reader why we're making this > particular check here. Hmm, that is a good idea... we should do it across all of messenger.c at once, though. Thanks! sage > > > + if (ret < 0) { > > + pr_err("%s%lld %s bad authorizer reply, failed to" > > + " authenticate server\n", > > + ENTITY_NAME(con->peer_name), > > + ceph_pr_addr(&con->peer_addr.in_addr)); > > + con->error_msg = "failed to authenticate server"; > > + return -1; > > + } > > + } > > + > > switch (con->in_reply.tag) { > > case CEPH_MSGR_TAG_FEATURES: > > pr_err("%s%lld %s feature set mismatch," > > > > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
