[Isms] Draft-narayan-isms-sshsm-radius-01 review

["David Harrington" <[e-mail]>]

Hi,

I have reviewed this document and have some general and some specific
suggestions. Overall, I think the document is progressing, but needs
to be more consistent with other isms documents, and could use a
restructuring.

General:
I recommend restructuring the document based on what is to be
supported, in an incremental manner:
1) "integration with legacy RADIUS" which would only deal with
authenticating the user and authorizing an SSH subsystem, and not deal
with integration with an ACM (or make it implementation-dependent to
hang onto authz attributes received during a legacy authn).

2) "integration with Dynamic RADIUS" which would support authorization
for a SSH subsystem PLUS authorize-only support in a RADIUS-specific
ACM. 

I suggest that the RADIUS state attribute established during authn be
passed as the securityName, since that is what the authorize-only
authz is tied to. 

To maintain modularity, an SSH-RADIUS transport model should pass the
RADIUS state attribute in the tmStateReference as the proposed
securityName, and the Transport Security Model would translate that
into the securityName. A RADIUS-specific ACM would then use the
securityName (RADIUS state attr) to attempt an authorize-only
access-request, and use the returned attributes to determine what
access should be allowed. If the securityname != a known
RADIUS-state-attr, then RADIUS would return an access-reject.

3) "integration with the proposed management policy attributes". Used
with authorize-only, RADIUS could return the new mgmt-policy-id
attribute to be used to create a mapping from securityname
(Radius-state-attr) to a "named" mgmt policy.

---------Specific comments---------

Introduction
as noted previously, there is no SSH SM, only an SSH TM. This should
be fixed throughout the document.

section2 s/(SSHSM) [sshsm]/[sshsm]/
	s/Security Model/Transport Model/
	"SSHSM requires ..." should be rewritten to better reflect
current isms documents. I am willign to help rewrite this if you want.
	s/explicitly//

Section 3.1
I recommend moving the sentence starting "SSH integration" before the
sentence starting "RADIUS will indicate"

Section 3.2
s/support multiple/supports multiple/
s/used SSH/used by SSH/
s/clients,/clients;/
The sentence starting "the 'password' method" is not needed in this
document; it is irrelevant to SNMP integration.
The sentence starting "SSH server implementations tyically" is not
needed.
s/rely of implementation/rely on implementation/

3.3
My mailer won't let me tell you that you have "the" mispelled as "teh"
;-)
The paragraph starting "RADIUS Servers will usually" could be
condensed.
The sentence starting [radman] should start a new paragragh.

3.3.1 
s/vlaue/value/

3.3.2
What happens if RADIUS has authorization attributes from the
access-accept saved and passed in tmStateRef provided by the transport
model, but the security model (USM/community/etc.) does not use the
authorized user (securityName) from the tmStateRef?

The sentence starting "The User-based security mdoel integrates with
VACM" is incorrect; USM integrates with the RFC3411 architecture,
which integrates with VACM.

The descritpotrion of the use of a local database is not consistent
with the RFC3411 descriotio of this usage.

This section talks about the proposed new mgmt-policy attributes, but
we should try ot make th epoprosal not dependent on that proposal. See
my general comments above.

s/comprises a policy or group name/comprises a policy, e.g., a group
name,/
s/mapping of the group name to/mapping of th epolicy name to/
"unlikely to change" and "likely to be based" are based on what
experience?
s/This mapping mechanisms is implementation specific/This mapping
mechanisms is model- and implementation-specific/

3.3.3
s/, at the most simple level,//
s/RADIUS clients, with the NAS, initiates/A RADIUS client within the
NAS initiates/

The sentence starting "In some cases" is not needed.
The sentence starting "In many deployements" should start a new
paragraph

3.3.4
TACACS needs a citation/reference
s/separate out/separate/

3.4
See general comments about securityname = RADIUS state attr

3.5
Sentence about accounting packets seems out of place; this should be
dropped or expalined further.

3.6 discussion of PAM is not needed. That is purely
implementation-specific and we do not discuss implementation issues.

"there is no equivalent shell" - is an SSH subsystem comparable to a
shell?

"The ability of the SSH server to use  ..." is this sentence needed?

s/such as PAM, provide/such as pAM, may provide/

"Transport Mapping Security Model" is no longer valid.
"It is possible therefore," is inconsistent with the architecure,
since the ACM does not get passed the tmStateRef.

"make calls to the RADIUS client" - would this be clearer as "make
calls to the RADIUS server"?

5.
s/amy/may/

7.
[sshsm] is incorrect.


David Harrington
[e-mail] 
[e-mail]
[e-mail]



_______________________________________________
Isms mailing list
[e-mail]
https://www1.ietf.org/mailman/listinfo/isms