Skip to content

NMS-16237: add support for RFC2348 "TFTP blksize" to the tftp implementation #8184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dino2gnt wants to merge 6 commits into
base: release-35.x
Choose a base branch
from
Open

NMS-16237: add support for RFC2348 "TFTP blksize" to the tftp implementation #8184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
197b8a8
NMS-16237: add support for RFC2348 "TFTP blksize" to the tftp server …
dino2gnt Dec 6, 2025
c235110
Fix test
dino2gnt Dec 6, 2025
17583af
Update features/device-config/tftp/src/main/java/org/opennms/features…
dino2gnt Dec 8, 2025
754040e
formatting
dino2gnt Dec 8, 2025
abc6976
Check state of previously opened socket before opening a new socket o…
dino2gnt Dec 8, 2025
c413f49
also remove generated filenamesuffix token
dino2gnt Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ...trieval/src/main/java/org/opennms/features/deviceconfig/retrieval/impl/RetrieverImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ public void onTimeout(CompletableFuture<Either<Failure, Success>> future) {

@Override
public void onFileReceived(InetAddress address, String fileName, byte[] content) {
if (fileName.endsWith(fileNameSuffix)) {
if (fileName.endsWith(fileNameSuffix) || fileName.contains(fileNameSuffix)) {
// it is unlikely, that the file receiver receives a file (with matching filename!) before the file
// upload was triggered
// -> just to be sure check that the future is set
Expand All @@ -224,9 +224,12 @@ public void onFileReceived(InetAddress address, String fileName, byte[] content)
catch (Exception e) {}
}
// strip the '.' and filenameSuffix from the filename
future.complete(Either.right(new Success(content, fileName.substring(0, fileName.length() - fileNameSuffix.length()), scriptOutput)));
future.complete(Either.right(new Success(content, fileName.replace(fileNameSuffix, ""), scriptOutput)));
}
}
else {
LOG.warn("Received file from {} without a matching 'filenameSuffix' token; Ignoring...", address.getHostAddress());
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...val/src/test/java/org/opennms/features/deviceconfig/retrieval/impl/RetrieverImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ public void shouldIgnoreOtherFiles() throws Exception {
var filenameSuffix = (String)vars.get("filenameSuffix");

// signal the receiver of some incoming file that has a different name
receiver.onFileReceived(InetAddress.getLocalHost(), "config.gz" + filenameSuffix + ".other", new byte[] { 1, 2, 3 });
receiver.onFileReceived(InetAddress.getLocalHost(), "config.gz.somerandomstringhuehuehue.other", new byte[] { 1, 2, 3 });

await().until(future::isDone);

Expand Down
89 changes: 83 additions & 6 deletions ...config/tftp/src/main/java/org/opennms/features/deviceconfig/tftp/impl/TftpServerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,21 @@
package org.opennms.features.deviceconfig.tftp.impl;

import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.IOException;
import java.io.OutputStream;
import java.net.DatagramPacket;
import java.net.DatagramSocket;
import java.net.InetAddress;
import java.net.InetSocketAddress;
import java.net.SocketException;
import java.net.SocketTimeoutException;
import java.net.UnknownHostException;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;

import org.apache.commons.net.io.FromNetASCIIOutputStream;
Expand All @@ -60,6 +63,7 @@
// - received files are not stored on disk but in memory and are handed off to registered listeners
// - the maximum file size to receive can be configured
// - requesting files is not supported
// - added support for RFC2348 blksize

public class TftpServerImpl implements TftpServer, Runnable, AutoCloseable {

Expand Down Expand Up @@ -92,6 +96,8 @@ private void handleWrite(final TFTPWriteRequestPacket twrp) throws IOException,
try {
int lastBlock = 0;
final String fileName = twrp.getFilename();
int negotiatedBlksize = TFTPDataPacket.MAX_DATA_LENGTH;
Map<String,String> twrpOptions = twrp.getOptions();

try {
if (twrp.getMode() == TFTP.NETASCII_MODE) {
Expand All @@ -104,9 +110,58 @@ private void handleWrite(final TFTPWriteRequestPacket twrp) throws IOException,
.getPort(), TFTPErrorPacket.UNDEFINED, e.getMessage()));
return;
}

TFTPAckPacket lastSentAck = new TFTPAckPacket(twrp.getAddress(), twrp.getPort(), 0);
sendData(transferTftp_, lastSentAck); // send the data

// Handle RFC2348 TFTP blksize
if (twrpOptions.containsKey("blksize")) {
LOG.debug("Received blksize option from client '{}', attempting negotiation", twrp.getAddress());
try {
negotiatedBlksize = Integer.parseInt(twrpOptions.get("blksize"));
if (negotiatedBlksize < 8 || negotiatedBlksize > 65464) {
LOG.error("Received invalid blksize '{}' option from client {}", negotiatedBlksize, twrp.getAddress());
transferTftp_.bufferedSend(new TFTPErrorPacket(twrp.getAddress(),
twrp.getPort(), TFTPErrorPacket.INVALID_OPTIONS_VALUE,
"Invalid blksize"));
return;
} else {
LOG.debug("Negotiating tftp blksize '{}' with client '{}'", negotiatedBlksize, twrp.getAddress());
// Create an OAck response packet - commons doesn't provide a method for this yet.
DatagramPacket oackPacket = createBlksizeOAckPacket(negotiatedBlksize, twrp.getAddress(), twrp.getPort());

try {
int localPort = transferTftp_.getLocalPort();
transferTftp_.close();
while (true) {
Copy link
Contributor

@cgorantla cgorantla Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think we should while(true) loops.

Let me try this differently.

Copy link
Contributor Author

@dino2gnt dino2gnt Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't feel good about it either.

if (!transferTftp_.isOpen()) { // wait until we know the socket is not open
break;
}
}
DatagramSocket socket = new DatagramSocket(localPort);
socket.send(oackPacket);
socket.close();
while (true) {
if (socket.isClosed()) { // wait until we know the socket is closed
break;
}
}
transferTftp_.open(localPort); // every day I'm shufflin'
} catch (Exception e) {
LOG.debug("Error sending OACK packet in response to 'blksize={}',", negotiatedBlksize, e);
return;
}
transferTftp_.resetBuffersToSize(negotiatedBlksize);
LOG.debug("Successfully negotiated blksize '{}' with client '{}'", negotiatedBlksize, twrp.getAddress());
}
} catch (final Exception e) {
LOG.error("Received invalid blksize '{}' option from client {}:", negotiatedBlksize, twrp.getAddress(), e);
transferTftp_.bufferedSend(new TFTPErrorPacket(twrp.getAddress(),
twrp.getPort(), TFTPErrorPacket.INVALID_OPTIONS_VALUE,
"Invalid blksize"));
return;
}
} else {
sendData(transferTftp_, lastSentAck); // send the ack
}

while (true) {
// get the response - ensure it is from the right place.
Expand Down Expand Up @@ -134,11 +189,12 @@ private void handleWrite(final TFTPWriteRequestPacket twrp) throws IOException,
dataPacket = transferTftp_.bufferedReceive();
} catch (final SocketTimeoutException e) {
statistics.incErrors();
LOG.error("did not receive data packet", e);
LOG.error("did not receive data packet from client '{}': ", twrp.getAddress(), e);
if (timeoutCount >= maxTimeoutRetries_) {
throw e;
}
// It didn't get our ack. Resend it.
LOG.debug("Resending missed ack to '{}'", twrp.getAddress());
transferTftp_.bufferedSend(lastSentAck);
timeoutCount++;
continue;
Expand All @@ -147,6 +203,7 @@ private void handleWrite(final TFTPWriteRequestPacket twrp) throws IOException,

if (dataPacket instanceof TFTPWriteRequestPacket) {
// it must have missed our initial ack. Send another.
LOG.debug("Resending WRQ ack to '{}'", twrp.getAddress());
lastSentAck = new TFTPAckPacket(twrp.getAddress(), twrp.getPort(), 0);
transferTftp_.bufferedSend(lastSentAck);
} else if (dataPacket == null || !(dataPacket instanceof TFTPDataPacket)) {
Expand All @@ -161,6 +218,7 @@ private void handleWrite(final TFTPWriteRequestPacket twrp) throws IOException,
final byte[] data = ((TFTPDataPacket) dataPacket).getData();
final int dataLength = ((TFTPDataPacket) dataPacket).getDataLength();
final int dataOffset = ((TFTPDataPacket) dataPacket).getDataOffset();
LOG.debug("Processing block {} from client '{}'", block, twrp.getAddress());

if (block > lastBlock || lastBlock == 65535 && block == 0) {
// it might resend a data block if it missed our ack
Expand All @@ -184,8 +242,9 @@ private void handleWrite(final TFTPWriteRequestPacket twrp) throws IOException,
}

lastSentAck = new TFTPAckPacket(twrp.getAddress(), twrp.getPort(), block);
LOG.debug("Sending Ack for block {} from client '{}'", block, twrp.getAddress());
sendData(transferTftp_, lastSentAck); // send the data
if (dataLength < TFTPDataPacket.MAX_DATA_LENGTH) {
if (dataLength < negotiatedBlksize) {
// end of stream signal - The transfer is complete.
bos.close();

Expand All @@ -195,7 +254,7 @@ private void handleWrite(final TFTPWriteRequestPacket twrp) throws IOException,

List<TftpFileReceiver> rs;
synchronized (receivers) {
rs = new ArrayList(receivers);
rs = new ArrayList<>(receivers);
}
rs.forEach(r -> {
r.onFileReceived(twrp.getAddress(), fileName, content);
Expand All @@ -209,6 +268,7 @@ private void handleWrite(final TFTPWriteRequestPacket twrp) throws IOException,
} catch (final SocketTimeoutException e) {
// this is the expected route - the client
// shouldn't be sending any more packets.
LOG.debug("tftp transfer from client '{}' complete.", twrp.getAddress());
break;
}

Expand Down Expand Up @@ -236,6 +296,7 @@ private void handleWrite(final TFTPWriteRequestPacket twrp) throws IOException,
}

// all done.
LOG.debug("Timing out transfer from client '{}'", twrp.getAddress());
break;
}
}
Expand Down Expand Up @@ -266,6 +327,7 @@ public void run() {
.getPort(), TFTPErrorPacket.ILLEGAL_OPERATION,
"Read not allowed by server."));
} else if (tftpPacket_ instanceof TFTPWriteRequestPacket) {
LOG.debug("Received write request from client '{}'", tftpPacket_.getAddress());
handleWrite((TFTPWriteRequestPacket) tftpPacket_);
} else {
statistics.incWarnings();
Expand Down Expand Up @@ -599,4 +661,19 @@ public TftpStatistics getStatistics() {
public TftpStatistics getAndResetStatistics() {
return statistics.cloneAndReset();
}

private DatagramPacket createBlksizeOAckPacket(int size, InetAddress addr, int port) throws IOException {
// build the oack packet - commons doesn't provide a method for this yet...
ByteArrayOutputStream oack = new ByteArrayOutputStream();
oack.write(0);
oack.write(TFTPPacket.OACK);
oack.write("blksize".getBytes(StandardCharsets.US_ASCII));
oack.write(0);
oack.write(String.valueOf(size).getBytes(StandardCharsets.US_ASCII));
oack.write(0);
//make it a byte array
byte[] oackData = oack.toByteArray();

return new DatagramPacket(oackData, oackData.length, addr, port);
}
}
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1770,7 +1770,7 @@
<commonsLangVersion>2.6</commonsLangVersion>
<commonsLang3Version>3.16.0</commonsLang3Version>
<commonsMath3Version>3.6.1</commonsMath3Version>
<commonsNetVersion>3.10.0</commonsNetVersion>
<commonsNetVersion>3.12.0</commonsNetVersion>
<commonsPoolVersion>1.6</commonsPoolVersion>
<commonsPool2Version>2.4.3</commonsPool2Version>
<commonsTextVersion>1.11.0</commonsTextVersion>
Expand Down