tpalfy commented on a change in pull request #5088: URL: https://github.com/apache/nifi/pull/5088#discussion_r681074327
########## File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/dto/UserDetails.java ########## @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.snmp.dto; + +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.nifi.snmp.utils.SNMPUtils; +import org.snmp4j.smi.OID; +import org.snmp4j.smi.OctetString; + +public class UserDetails { + + private final OctetString securityName; + private final OID authProtocol; + private final OctetString authPassphrase; + private final OID privProtocol; + private final OctetString privPassphrase; + + public UserDetails(@JsonProperty("securityName") final String securityName, Review comment: Are the `@JsonProperty` annotations necessary? ########## File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/operations/SNMPResourceHandler.java ########## @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.snmp.operations; + +import org.apache.nifi.snmp.exception.CloseSNMPClientException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.snmp4j.Snmp; +import org.snmp4j.Target; +import org.snmp4j.security.SecurityModels; +import org.snmp4j.smi.Integer32; + +import java.io.IOException; + +public class SNMPResourceHandler implements AutoCloseable { Review comment: Shouldn't be an `AutoCloseable` if doesn't behave like one. Makes harder to see where `close()` is actually called from. ########## File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/exception/CreateSNMPClientException.java ########## @@ -18,8 +18,8 @@ public class CreateSNMPClientException extends SNMPException { - public CreateSNMPClientException(final String errorMessage) { - super(errorMessage); + public CreateSNMPClientException(final Exception exception) { Review comment: Unless there is a good reason we usually want to throw ProcessException (or IOException) from processors o controller services (instead of an extension-specific one). (Note: called from `AbstractSNMPProcessor.initSnmpManager` and `ListenTrapSNMP.initSnmpManager`) ########## File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/processors/properties/V3SecurityProperties.java ########## @@ -0,0 +1,126 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.snmp.processors.properties; + +import org.apache.nifi.components.AllowableValue; +import org.apache.nifi.components.PropertyDescriptor; +import org.apache.nifi.processor.util.StandardValidators; + +import static org.apache.nifi.snmp.processors.properties.BasicProperties.SNMP_V3; +import static org.apache.nifi.snmp.processors.properties.BasicProperties.SNMP_VERSION; + +public class V3SecurityProperties { + + private static final String SHA_2_ALGORITHM = "Provides authentication based on the HMAC-SHA-2 algorithm."; + + private static final String AES_DESCRIPTION = "AES is a symmetric algorithm which uses the same 128, 192, or 256 bit" + + " key for both encryption and decryption (the security of an AES system increases exponentially with key length)."; + + // SNMPv3 security levels + public static final AllowableValue NO_AUTH_NO_PRIV = new AllowableValue("noAuthNoPriv", "noAuthNoPriv", + "No authentication or encryption."); + public static final AllowableValue AUTH_NO_PRIV = new AllowableValue("authNoPriv", "authNoPriv", + "Authentication without encryption."); + public static final AllowableValue AUTH_PRIV = new AllowableValue("authPriv", "authPriv", + "Authentication and encryption."); Review comment: ```suggestion public static final AllowableValue NO_AUTH_NO_PRIV = new AllowableValue(SecurityLevel.noAuthNoPriv.name(), SecurityLevel.noAuthNoPriv.name(), "No authentication or encryption."); public static final AllowableValue AUTH_NO_PRIV = new AllowableValue(SecurityLevel.authNoPriv.name(), SecurityLevel.authNoPriv.name(), "Authentication without encryption."); public static final AllowableValue AUTH_PRIV = new AllowableValue(SecurityLevel.authPriv.name(), SecurityLevel.authPriv.name(), "Authentication and encryption."); ``` ########## File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/factory/core/V3SNMPFactory.java ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.snmp.factory.core; + +import org.apache.nifi.snmp.configuration.SNMPConfiguration; +import org.apache.nifi.snmp.utils.SNMPUtils; +import org.snmp4j.Snmp; +import org.snmp4j.Target; +import org.snmp4j.UserTarget; +import org.snmp4j.mp.MPv3; +import org.snmp4j.security.SecurityLevel; +import org.snmp4j.security.SecurityModels; +import org.snmp4j.security.SecurityProtocols; +import org.snmp4j.security.USM; +import org.snmp4j.security.UsmUser; +import org.snmp4j.smi.OID; +import org.snmp4j.smi.OctetString; + +import java.util.Optional; + +public class V3SNMPFactory extends SNMPManagerFactory implements SNMPContext { + + @Override + public Snmp createSnmpManagerInstance(final SNMPConfiguration configuration) { + final Snmp snmpManager = super.createSnmpManagerInstance(configuration); + + // Create USM. + final OctetString localEngineId = new OctetString(MPv3.createLocalEngineID()); + final USM usm = new USM(SecurityProtocols.getInstance(), localEngineId, 0); + SecurityModels.getInstance().addSecurityModel(usm); + + final OID authProtocol = Optional.ofNullable(configuration.getAuthProtocol()) + .map(SNMPUtils::getAuth).orElse(null); + final OctetString authPassphrase = Optional.ofNullable(configuration.getAuthPassphrase()) + .map(OctetString::new).orElse(null); + final OID privacyProtocol = Optional.ofNullable(configuration.getPrivacyProtocol()) + .map(SNMPUtils::getPriv).orElse(null); + final OctetString privacyPassphrase = Optional.ofNullable(configuration.getPrivacyPassphrase()) + .map(OctetString::new).orElse(null); + + // Add user information. + Optional.ofNullable(configuration.getSecurityName()) + .map(OctetString::new) + .ifPresent(securityName -> addUser(snmpManager, securityName, authProtocol, authPassphrase, privacyProtocol, privacyPassphrase)); + + return snmpManager; + } + + private void addUser(final Snmp snmpManager, final OctetString securityName, final OID authProtocol, final OctetString authPassphrase, + final OID privacyProtocol, final OctetString privacyPassphrase) { + snmpManager.getUSM().addUser(securityName, new UsmUser(securityName, authProtocol, authPassphrase, + privacyProtocol, privacyPassphrase)); + } Review comment: This part is fairly confusing. According to the code we add a user only if `configuration.getSecurityName()` is set. However, is that a valid use case? If it is, I think the following would convey this a bit better: ```suggestion // Add user information. Optional.ofNullable(configuration.getSecurityName()) .map(OctetString::new) .ifPresent(securityName -> { OID authProtocol = Optional.ofNullable(configuration.getAuthProtocol()) .map(SNMPUtils::getAuth).orElse(null); OctetString authPassphrase = Optional.ofNullable(configuration.getAuthPassphrase()) .map(OctetString::new).orElse(null); OID privacyProtocol = Optional.ofNullable(configuration.getPrivacyProtocol()) .map(SNMPUtils::getPriv).orElse(null); OctetString privacyPassphrase = Optional.ofNullable(configuration.getPrivacyPassphrase()) .map(OctetString::new).orElse(null); snmpManager.getUSM().addUser(securityName, new UsmUser(securityName, authProtocol, authPassphrase, privacyProtocol, privacyPassphrase)); }); return snmpManager; } ``` ########## File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/exception/CloseSNMPManagerException.java ########## @@ -14,17 +14,12 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.nifi.snmp.factory; +package org.apache.nifi.snmp.exception; -import org.apache.nifi.snmp.configuration.SNMPConfiguration; -import org.snmp4j.Snmp; -import org.snmp4j.Target; +public class CloseSNMPManagerException extends SNMPException { Review comment: Unless there is a good reason we usually want to throw `ProcessException` (or `IOException`) from processors o controller services (instead of an extension-specific one). (Note: called from `AbstractSNMPProcessor.close`) ########## File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/factory/core/SNMPManagerFactory.java ########## @@ -14,27 +14,29 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.nifi.snmp.factory; +package org.apache.nifi.snmp.factory.core; import org.apache.nifi.snmp.configuration.SNMPConfiguration; +import org.apache.nifi.snmp.exception.CreateSNMPClientException; import org.snmp4j.Snmp; -import org.snmp4j.Target; -import org.snmp4j.mp.SnmpConstants; +import org.snmp4j.smi.UdpAddress; +import org.snmp4j.transport.DefaultUdpTransportMapping; -public class V2cSNMPFactory extends AbstractSNMPFactory implements SNMPFactory { +import java.io.IOException; - @Override - public boolean supports(final int version) { - return SnmpConstants.version2c == version; - } +public class SNMPManagerFactory { Review comment: If the port is enough to open a port on localhost I'd suggest the following: ```java public class SNMPManagerFactory { public Snmp createSnmpManagerInstance(final SNMPConfiguration configuration) { try { final Snmp snmpManager = new Snmp(new DefaultUdpTransportMapping(new UdpAddress(configuration.getManagerPort()); snmpManager.listen(); return snmpManager; } catch (IOException e) { throw new CreateSNMPClientException(e); } } } ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
