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. > + 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. > + 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
