From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Jarno Malmari Newsgroups: gmane.emacs.devel Subject: Re: [PATCH 1/2] Refactor digest authentication in url-auth Date: Sun, 13 Nov 2016 23:57:08 +0200 Message-ID: <87shqvqc0b.fsf@malmari.fi> References: <1478988194-7761-1-git-send-email-jarno@malmari.fi> <1479036981-22047-1-git-send-email-jarno@malmari.fi> <1479036981-22047-2-git-send-email-jarno@malmari.fi> <83d1hzcr5r.fsf@gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1479074266 24481 195.159.176.226 (13 Nov 2016 21:57:46 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 13 Nov 2016 21:57:46 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) Cc: emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Nov 13 22:57:41 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c62mD-0003sO-Ke for ged-emacs-devel@m.gmane.org; Sun, 13 Nov 2016 22:57:25 +0100 Original-Received: from localhost ([::1]:35180 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c62mG-0008Hn-K7 for ged-emacs-devel@m.gmane.org; Sun, 13 Nov 2016 16:57:28 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:59627) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c62m9-0008HV-DY for emacs-devel@gnu.org; Sun, 13 Nov 2016 16:57:22 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c62m8-0007fA-2s for emacs-devel@gnu.org; Sun, 13 Nov 2016 16:57:21 -0500 Original-Received: from out4-smtp.messagingengine.com ([66.111.4.28]:45327) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c62m4-0007eV-62; Sun, 13 Nov 2016 16:57:16 -0500 Original-Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 1939220371; Sun, 13 Nov 2016 16:57:14 -0500 (EST) Original-Received: from frontend1 ([10.202.2.160]) by compute1.internal (MEProxy); Sun, 13 Nov 2016 16:57:14 -0500 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=malmari.fi; h=cc :content-type:date:from:in-reply-to:message-id:mime-version :references:subject:to:x-me-sender:x-me-sender:x-sasl-enc :x-sasl-enc; s=mesmtp; bh=Wt8vduv+81Svy0ltJdubnIWz0PE=; b=n9CalB AiZ6Xz69EKMbvOv/L3WFtcR+t9RsFHtGJtsEe5T8U50Kx9X8F9R7tW3WMi7jY9k1 VFjoEEq+5PXbq67QZT+mJAHn0Yz79CUt2qCcndWPvpPKbrJeIuSRj2VSSWcZK9i2 TMCH8LEs9XEqtAb/nbFaFb1Ww9aC+0sK0/cxk= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc:x-sasl-enc; s=smtpout; bh=Wt8vduv+81Svy0 ltJdubnIWz0PE=; b=MI15ooUi/kmdsF3DU6/N79Yun8mnMdEsvkmAXfTUc1GAmA 5ZwspCslmyPrDzFWAeV+vCxvvsa8ADh6PHHVeOJrweOKQ9b4lQEtGJpkKBSWK2/m dqPy/ef20s54LlqWspPN8aSl69FPzeCOpsniNy2KTAkTLdH7MsY8b3+h7fQYw= X-ME-Sender: X-Sasl-enc: WTq7HyJPMcM5hOWkV1+4Wp0aiceECf6R7/1HZ4e4Ibig 1479074229 Original-Received: from vabi (a88-113-156-118.elisa-laajakaista.fi [88.113.156.118]) by mail.messagingengine.com (Postfix) with ESMTPA id 3B2557E05C; Sun, 13 Nov 2016 16:57:09 -0500 (EST) In-Reply-To: <83d1hzcr5r.fsf@gnu.org> (Eli Zaretskii's message of "Sun, 13 Nov 2016 17:53:36 +0200") X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 66.111.4.28 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:209376 Archived-At: Eli Zaretskii writes: >> * lisp/url/url-auth.el (url-digest-auth, url-digest-auth-create-key): >> (url-digest-auth-build-response, url-digest-auth-directory-id-assoc, >> url-digest-auth-name-value-string, url-digest-auth-source-creds, >> url-digest-cached-key, url-digest-cache-key, url-digest-find-creds, >> url-digest-find-new-key, url-digest-prompt-creds): Add new functions to >> simplify code and aid in unit testing. > > This is not how we format commit logs of changes to several functions > whose list spans several lines. The entry should look like this: > > * lisp/url/url-auth.el (url-digest-auth, url-digest-auth-create-key): > (url-digest-auth-build-response, url-digest-auth-directory-id-assoc) > (url-digest-auth-name-value-string, url-digest-auth-source-creds) > (url-digest-cached-key, url-digest-cache-key, url-digest-find-creds) > (url-digest-find-new-key, url-digest-prompt-creds): Add new functions to > simplify code and aid in unit testing. > > IOW, each line begins is separately parenthesized. Will fix. Seems I wasn't able to deduce the correct format from previous log entries nor from the CONTRIBUTE file. Is this format obvious to others? Should the "each line separately parenthesized" be described in the CONTRIBUTE file? > >> +(defsubst url-digest-auth-kd (data secret) >> + "Apply digest algorithm MD5 to DATA using SECRET as described in RFC 2617." >> + (md5 (url-digest-auth-colonjoin secret data))) >> + >> +(defsubst url-digest-auth-make-ha1 (user realm password) >> + "Compute hashed A1 value as described in RFC 2617. >> +USER, REALM, and PASSWORD are strings used to create the hash >> +from." >> + (md5 (url-digest-auth-colonjoin user realm password))) > > Hear and elsewhere, I find the doc strings impenetrable without having > RFC 2617 around. Yeah, it's kind of annoying. > I wonder if it would make sense to describe the arguments in a bit > more detail, such that consulting the RFC each time these are used > would not be necessary. Is that practical? I'll give it a try and see if it gets messy. Some arguments might have complex(ish) BNF to them but we may not have to go that far in our limited implementation. > >> +(defun url-digest-auth-directory-id-assoc (dirkey keylist) >> + "Find the best match in key list using a path or a realm. >> + >> +The string DIRKEY is either a path or a realm. The key list to >> +search through is the alist KEYLIST where car of each element is >> +either a path or a realm. Realms are searched for an exact >> +match. For paths, an ancestor is sufficient for a match." > > GNU coding standards frown on using "path" for anything but PATH-style > directory lists. Please use "file name" or "directory name" instead. Good to know. Is there a convention to indicate path part in URIs? > >> + ;; no partial matches for non-path, i.e. realm >> + (and (string-match "/" dirkey) > > This will fail with Windows file names that use backslashes. We're actually not talking about file system paths. We're talking about paths (directory names?) in URIs which define, together with realm, the "protection space" (see RFC2617 ;)). The idea behind protection spaces are that the same credentials can be preemptively reused without bothering the user. I can't say if the url package is using the path part correctly or not, or if it's even trying to implement protection spaces with it. (I'm not an expert on this. I just refactored the code trying to preserve the old functionality the best I could.) In any case, url package uses either realm or the path as key to find cached credentials, and prefers the realm. If the path is used, then same credentials is tried for every "sub directory" resource: If path "/a/b" (www.example.com/a/b) has credentials cached, the same credentials are tried for "a/b/c" and so on. > >> +(defun url-digest-cached-key (url realm) >> + "Find best match for URL and REALM from `url-digest-auth-storage'. >> +The return value is a list consisting of a realm (or a directory) >> +a user name, and hashed authentication tokens HA1 and HA2, >> +defined in RFC 2617. Modifying the contents of the returned list > > Two spaces between sentences, please. Will fix. > >> + "Create a key for digest authentication method. >> +The USERNAME and PASSWORD are the credentials for REALM and are >> +used in making a hashed value named HA1. The HTTP METHOD and URI >> +makes a second hashed value HA2. These hashes are used in making >> +the authentication key that can be stored without saving the >> +password in plain text. The return value is a list consisting of >> +hashed authentication tokens HA1 and HA2, defined in RFC 2617. > > Same here. On to it. > >> +Primary method for finding credentials is from Emacs auth-source. >> +If password isn't found, and PROMPT is non-nil, allow to query >> +credentials via minibuffer. > > "Allow to query" or "query"? Query. Confusion arised since prompt=t by itself doesn't mean query will happen. Will fix. > >> + ;; if incomplete and prompt allowed, prompt the user > > Comments should begin with a capital letter and end with a period, as > normal sentences are (here and elsewhere in the patch). Does that implicitly mean that each comment should be a complete sentence? Granted, this one is close to one. Will fix. > > Thanks.